vigneshio commented on code in PR #4825:
URL: https://github.com/apache/polaris/pull/4825#discussion_r3443631340
##########
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:
@dimas-b @snazy
Thanks for the questions.
This came from code review alone.. I was scanning for places that could NPE
on the nullable root (it returns null if `!isResolveAllSucceeded()` or the
lookup itself fails, and there is a warn log). _I didn't have a prod stack
trace for this exact NPE, but List.of(null) will always fail if it hits._
You're right @dimas-b that returning an empty list for
`getResolvedRootContainerEntityAsPath()` isn't great — callers (root grants,
authorizer paths) expect the actual root entity. I've updated the code locally
to use `diagnostics.checkNotNull` there instead.
For the prepend cases I left the "skip if null" behaviour - that is the same
pattern `getResolvedTopLevelEntity `already uses.
On making the root method itself @NonNull or throw: that sounds like a
reasonable follow-up i feel, but for now the guard stops the immediate issue.
Should I push the update..
WDYT??
--
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]
