core: refactor `PolarisSecurable` with `PathSegment` by sungwy · Pull Request #4016 · apache/polaris

sungwy · 2026-03-18T04:28:26Z

While working on #3999 we realized that PolarisSecurable requires an Entity representation with its parent relationships and their corresponding EntityType.

This PR introduces a new record type PathSegment that has name and EntityType that represents a segment of the resource path, that can be ordered to construct a PolarisSecurable.

This is not yet a breaking change, because a release hasn't been made since PolarisSecurable was introduced.

Checklist

🛡️ Don't disclose security issues! (contact security@apache.org)
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 Added/updated tests with good coverage, or manually tested (and explained how)
💡 Added comments for complex logic
🧾 Updated CHANGELOG.md (if needed)
📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot

Pull request overview

This PR refactors PolarisSecurable to represent authorization targets as an ordered list of typed path segments, and updates authorization request logic/tests to use the new representation and validate basic hierarchy rules.

Changes:

Introduces PathSegment and changes PolarisSecurable to store List<PathSegment> with derived getLeaf()/getParents() plus validation.
Updates AuthorizationRequest.containsType to use the securable leaf segment type.
Updates/adds unit tests to construct securables via PathSegment paths and to assert validation behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java	Switches securable identity from `(entityType, nameParts)` to a fully-qualified `pathSegments` model with validation.
polaris-core/src/main/java/org/apache/polaris/core/auth/PathSegment.java	Adds a record to represent a single `(entityType, name)` segment in a resource path.
polaris-core/src/main/java/org/apache/polaris/core/auth/AuthorizationRequest.java	Updates type detection to look at the leaf segment’s entity type.
polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationRequestTest.java	Migrates tests to the new `PathSegment`-based `PolarisSecurable.of(...)` API and adds a validation test.
polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisSecurableTest.java	Adds unit tests for `getLeaf()`/`getParents()` and invalid hierarchy validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

polaris-core/src/test/java/org/apache/polaris/core/auth/PolarisSecurableTest.java

adutra · 2026-03-18T14:43:12Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

+    List<PathSegment> pathSegments = getPathSegments();
+    return pathSegments.get(pathSegments.size() - 1);


Suggested change

List<PathSegment> pathSegments = getPathSegments();

return pathSegments.get(pathSegments.size() - 1);

return getPathSegments().getLast();

Looks like we are still compiling at 17 with polaris-client:

polaris/build-logic/src/main/kotlin/polaris-client.gradle.kts

Line 24 in f39a9a3

tasks.withType(JavaCompile::class.java).configureEach { options.release = 17 }

Maybe we can keep this PR with the current syntax and raise a DISCUSS thread to assess moving polaris-client compilation to 21?

Java 21 discussion is on-going (although slowly 😅 ): https://lists.apache.org/thread/v8ghm3k5nh9xp9x0xdo2kknk5ph0rnmb

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

adutra · 2026-03-18T14:47:38Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

+    for (PathSegment parent : getParents()) {
+      Preconditions.checkState(
+          parent.entityType() != PolarisEntityType.ROOT,
+          "PathSegments must not include ROOT for securable leaf=%s",


I'm not following, shouldn't the highest ancestor path segments be allowed to be ROOT for some securables?

I'm rooting for removing ROOT in the parent chain in PolarisSecurable, because including ROOT as a parent is a strange RBAC specific semantic. I don't think external PolarisAuthorizer implementations like OPA or Ranger would care about a ROOT parent.

I agree that ROOT should not be a factor in authZ decisions in general. Existing native RBAC uses it, which is fine... historical reasons.

It's like appending . to DNS names - it makes them formally absolute, but it makes no practical difference in most cases 😅

Let's standardize on starting paths at the catalog in AuthZ SPI.

adutra · 2026-03-18T14:48:26Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

+          "PathSegments must not include ROOT for securable leaf=%s",
+          getLeaf());
+    }
+    if (getLeaf().entityType().isTopLevel()) {


I think we are missing unit tests for some of the validation failures.

Happy to add more coverage

polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationRequestTest.java

dimas-b · 2026-03-19T23:23:32Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

+  default PathSegment getLeaf() {
+    List<PathSegment> pathSegments = getPathSegments();
+    Preconditions.checkState(
+        !pathSegments.isEmpty(), "PathSegments must contain at least one segment");


This check is a bit pointless, given the validate() method 🤔

I agree, but I thought of just keeping this to make the error more descriptive in the future, if our API evolves... it shouldn't ever happen and PathSegments must contain at least one segment feels more descriptive than ArrayIndexOutOfBoundsException.

well, I do not see how this check can ever fail 🤔 I believe the failure will happen in the validate() method and the object will never get constructed for this method to be called on 🤔

dimas-b

LGTM overall... with minor comments 👍

dimas-b · 2026-03-19T23:24:57Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PolarisSecurable.java

+    Preconditions.checkState(
+        getPathSegments().get(0).entityType().isTopLevel(),
+        "PathSegments must start with a top-level entity");
+    for (PathSegment parent : getParents()) {


Why check only parents? I guess ROOT is not meaningful as a leaf element either 🤔

yeah I guess we could expand it to the leaf as well

dimas-b · 2026-03-19T23:27:16Z

polaris-core/src/test/java/org/apache/polaris/core/auth/AuthorizationRequestTest.java

            List.of(
                AuthorizationTargetBinding.of(
-                    PolarisSecurable.of(PolarisEntityType.PRINCIPAL, List.of("alice")), null)));
+                    PolarisSecurable.of(new PathSegment(PolarisEntityType.PRINCIPAL, "alice")),


nit: maybe add a convenience .of() method for this? It looks like a common pattern.

that's a nice neat trick :)

hmm I thought about this a bit more, and I'm thinking of just keeping the plain constructors for now. I think it's explicit and consistent for securables with single PathSegment and multiple segments

ok, no worries

dimas-b · 2026-03-19T23:28:50Z

polaris-core/src/main/java/org/apache/polaris/core/auth/PathSegment.java

+import org.apache.polaris.core.entity.PolarisEntityType;
+
+/** One segment in a fully qualified resource path. */
+public record PathSegment(PolarisEntityType entityType, String name) {


nit: maybe add a convenience .pathSegment() builder method? Callers would be able to statically import it and have less verbose code.

refactor PolarisSecurable with PathSegment

23c3016

Copilot AI review requested due to automatic review settings March 18, 2026 04:28

github-project-automation bot added this to Basic Kanban Board Mar 18, 2026

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Mar 18, 2026

Copilot started reviewing on behalf of sungwy March 18, 2026 04:29 View session

Copilot AI reviewed Mar 18, 2026

View reviewed changes

adopt feedback from copilot

90c940a

adutra reviewed Mar 18, 2026

View reviewed changes

adopt feedback - thanks alex

eb7ffa6

sungwy requested a review from adutra March 19, 2026 01:38

sungwy mentioned this pull request Mar 19, 2026

Tracking Issue: Refactor PolarisAuthorizer SPI to skip native Principal and RBAC resolution requirement in Polaris #3779

Open

dimas-b reviewed Mar 19, 2026

View reviewed changes

dimas-b previously approved these changes Mar 19, 2026

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Mar 19, 2026

adopt nits

6f6c69d

sungwy dismissed dimas-b’s stale review via 6f6c69d March 20, 2026 02:33

		List<PathSegment> pathSegments = getPathSegments();
		return pathSegments.get(pathSegments.size() - 1);

	List<PathSegment> pathSegments = getPathSegments();
	return pathSegments.get(pathSegments.size() - 1);
	return getPathSegments().getLast();

Conversation

sungwy commented Mar 18, 2026

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Mar 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dimas-b Mar 19, 2026 •

edited

Loading

dimas-b left a comment •

edited

Loading