dimas-b commented on code in PR #4825:
URL: https://github.com/apache/polaris/pull/4825#discussion_r3443265382
##########
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:
I'd prefer a strong check for the value to be non-null in this case.
The other related call paths are a bit convoluted :sweat_smile: I did not
review it exhaustively, but in general `getResolvedRootContainerEntity()`
appears to return `null` only when the overall resolution process did not
succeed. In this case, I do not see a compelling reason for caller to actually
invoke `getResolvedRootContainerEntity()`. They should error out earlier.
All in all, I tend to think `getResolvedRootContainerEntity()` should return
a `@NonNull` value or throw.
WDYT?
Did you hit the NPE in this case in practice, or is this PR based purely on
core review?
--
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]
