Skip to content

fix(frontend): align role rule merge semantics#24936

Open
ouyuanning wants to merge 1 commit into
matrixorigin:3.0-devfrom
ouyuanning:improve-mo-from-24792-3.0-dev
Open

fix(frontend): align role rule merge semantics#24936
ouyuanning wants to merge 1 commit into
matrixorigin:3.0-devfrom
ouyuanning:improve-mo-from-24792-3.0-dev

Conversation

@ouyuanning

@ouyuanning ouyuanning commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #23271

What this PR does / why we need it:

This ports the relevant behavior from #24792 to 3.0-dev:

  • Merge compatible rewrite rules from multiple active roles by OR-ing WHERE filters instead of composing UNION queries.
  • Keep incompatible rewrite rules deterministic by letting later active-role rules override earlier ones.
  • Make SET SECONDARY ROLE ALL validate secondary role metadata without changing the current primary role.
  • Ensure SET ROLE switches the current role and invalidates privilege/rule caches.
  • Update unit tests and BVT cases for role rewrite rules, SET ROLE, and ownership privilege behavior.

Validation performed:

  • gofmt
  • git diff --check

Go test notes:

  • go test -mod=vendor ./pkg/frontend -run TestMergeRewriteRules is blocked by the local repository vendor mismatch between go.mod and vendor/modules.txt.
  • GOFLAGS=-mod=mod go test ... is blocked by the local environment missing usearch.h for github.com/unum-cloud/usearch/golang.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found one substantive issue in the new rewrite-rule merge behavior.

The current implementation now OR-merges many single-table SELECT rules as long as the projection list text matches, for example select a from db1.t1 where .... That is not semantics-preserving in general. It is only safe for row-identity-preserving projections (effectively select * / exact table-star forms). For partial projections like select a ..., two different base rows that satisfy different role predicates but project to the same a value will now both be returned, whereas the previous UNION DISTINCT-style behavior would have deduplicated the projected result.

What makes this higher confidence is that the new tests explicitly bless this for select a ... and select a as x .... So the PR is not just changing implementation shape — it is also locking in a user-visible semantic change for multi-role rewrite results. I think OR-merging should be restricted to projections that are guaranteed row-preserving, and non-identity projections should keep a semantics-preserving fallback instead of being treated as mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working kind/enhancement kind/test-ci size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants