vigneshio commented on code in PR #4825:
URL: https://github.com/apache/polaris/pull/4825#discussion_r3442189475
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/PolarisResolutionManifest.java:
##########
@@ -272,7 +272,10 @@ public Set<PolarisBaseEntity>
getAllActivatedPrincipalRoleEntities() {
}
public PolarisResolvedPathWrapper getResolvedRootContainerEntityAsPath() {
- return new
PolarisResolvedPathWrapper(List.of(getResolvedRootContainerEntity()));
+ ResolvedPolarisEntity root = getResolvedRootContainerEntity();
+ return root == null
+ ? new PolarisResolvedPathWrapper(List.of())
Review Comment:
Thanks @dimas-b for the review.
I checked the callers of `getResolvedRootContainerEntityAsPath():`
1. It is used for root-level operations (grant/revoke privilege on the root
container, basic root operations) and in `PolarisAuthorizerImpl` for
`TargetlessAuthorizationIntent` (when rooting is ROOT) and
RootPrivilegeGrantAuthorizationIntent. These sites take the wrapper and either
call getRawLeafEntity() or iterate the resolved path for grant checks.
2. The `prependRootContainer` paths (getResolvedPath(..., true) and
`getResolvedReferenceCatalogEntity(true))` just optionally add the root as a
parent prefix. In those cases we skip the root when it is absent (same pattern
already used in getResolvedTopLevelEntity).
The current PR change returns an empty path from
`getResolvedRootContainerEntityAsPath()` (to avoid List.of(null)).
Given that callers of this specific method appear to expect the root entity
to exist, would an explicit failure (eg. diagnostics.checkNotNull) be
preferable here instead of an empty path? or do you think we should keep the
empty path, return null and update the callers or handle it differently?
We will go with whatever approach makes sense and update the PR accordingly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
