Copilot commented on code in PR #3760:
URL: https://github.com/apache/polaris/pull/3760#discussion_r2806891258
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -238,6 +238,23 @@ public void addPath(@Nonnull ResolverPath path) {
* getResolvedXYZ() method can be called.
*/
public ResolverStatus resolveAll() {
+ return resolveWithPlan(ResolvePlan.all(referenceCatalogName));
+ }
+
+ /**
+ * Run the resolution process for a subset of resolvables.
+ *
+ * @param selections explicit selection of resolvables to resolve
+ * @return the status of the resolver. If success, the requested entities
have been resolved and
+ * the corresponding getResolvedXYZ() methods can be called.
+ */
+ public ResolverStatus resolveSelections(@Nonnull Set<Resolvable> selections)
{
+ diagnostics.checkNotNull(selections, "resolver_selections_is_null");
+ diagnostics.check(!selections.isEmpty(), "resolver_selections_is_empty");
+ return resolveWithPlan(ResolvePlan.fromSelections(selections,
referenceCatalogName));
Review Comment:
`resolveSelections(...)` can succeed without resolving some
previously-guaranteed fields (e.g., caller principal). In that case, getters
like `getResolvedCallerPrincipal()` (annotated `@Nonnull`) can return null
after a successful resolution, which is a nullness contract violation and can
trigger downstream NPEs. Consider tracking which components were resolved
(store the plan/selections) and have getters either throw if the component
wasn’t requested/resolved, or adjust the API/annotations to reflect optionality
for selection-based resolution.
```suggestion
// Currently, some getters are annotated @Nonnull and are expected to be
safe to call
// after a successful resolution. To avoid violating those contracts
when only a subset
// of resolvables is requested, we perform a full resolution here.
return resolveAll();
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -247,7 +264,7 @@ public ResolverStatus resolveAll() {
int count = 0;
ResolverStatus status;
do {
- status = runResolvePass();
+ status = runResolvePass(plan);
count++;
Review Comment:
The resolve-pass loop increments `count` twice per iteration (`count++`
inside the loop and `++count` in the while condition). This effectively halves
the intended max-pass limit and makes the counter harder to reason about.
Consider incrementing the counter in exactly one place (either inside the loop
body or in the condition).
```suggestion
```
##########
runtime/service/src/main/java/org/apache/polaris/service/auth/RequestAuthorizationState.java:
##########
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.service.auth;
+
+import jakarta.annotation.Nonnull;
+import jakarta.enterprise.context.RequestScoped;
+import java.util.concurrent.atomic.AtomicReference;
+import org.apache.polaris.core.auth.AuthorizationState;
+import org.apache.polaris.core.persistence.resolver.PolarisResolutionManifest;
+
+/**
+ * Request-scoped {@link AuthorizationState} implementation for runtime
services.
+ *
+ * <p>Holds a single {@link PolarisResolutionManifest} reference that must be
set once per request.
+ */
+@RequestScoped
+public class RequestAuthorizationState implements AuthorizationState {
+ private final AtomicReference<PolarisResolutionManifest> resolutionManifest =
+ new AtomicReference<>();
+
+ @Override
+ @Nonnull
+ public PolarisResolutionManifest getResolutionManifest() {
+ PolarisResolutionManifest manifest = resolutionManifest.get();
+ if (manifest == null) {
+ throw new IllegalStateException("AuthorizationState resolution manifest
is not set");
+ }
+ return manifest;
+ }
+
+ @Override
+ public void setResolutionManifest(@Nonnull PolarisResolutionManifest
resolutionManifest) {
+ if (!this.resolutionManifest.compareAndSet(null, resolutionManifest)) {
+ throw new IllegalStateException("AuthorizationState resolution manifest
already set");
+ }
Review Comment:
`setResolutionManifest(...)` allows a null value to be stored (annotations
aren’t enforced at runtime). That means the first call can succeed with null
and later `getResolutionManifest()` will fail with a misleading “not set”
error. Add an explicit null check (or use `Objects.requireNonNull`) before
`compareAndSet`.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -856,6 +888,68 @@ private ResolverStatus resolveReferenceCatalog(
return new ResolverStatus(ResolverStatus.StatusEnum.SUCCESS);
}
+ private static final class ResolvePlan {
+ private final boolean resolveCallerPrincipal;
+ private final boolean resolvePrincipalRoles;
+ private final boolean resolveReferenceCatalog;
+ private final boolean resolveCatalogRoles;
+ private final boolean resolveTopLevelEntities;
+ private final boolean resolvePaths;
+
+ private ResolvePlan(
+ boolean resolveCallerPrincipal,
+ boolean resolvePrincipalRoles,
+ boolean resolveReferenceCatalog,
+ boolean resolveCatalogRoles,
+ boolean resolveTopLevelEntities,
+ boolean resolvePaths) {
+ this.resolveCallerPrincipal = resolveCallerPrincipal;
+ this.resolvePrincipalRoles = resolvePrincipalRoles;
+ this.resolveReferenceCatalog = resolveReferenceCatalog;
+ this.resolveCatalogRoles = resolveCatalogRoles;
+ this.resolveTopLevelEntities = resolveTopLevelEntities;
+ this.resolvePaths = resolvePaths;
+ }
+
+ private static ResolvePlan all(@Nullable String referenceCatalogName) {
+ boolean hasReferenceCatalog = referenceCatalogName != null;
+ return new ResolvePlan(
+ true, true, hasReferenceCatalog, hasReferenceCatalog, true,
hasReferenceCatalog);
+ }
+
+ private static ResolvePlan fromSelections(
+ Set<Resolvable> selections, @Nullable String referenceCatalogName) {
+ boolean resolvePaths = selections.contains(Resolvable.REQUESTED_PATHS);
+ boolean resolveTopLevelEntities =
+ selections.contains(Resolvable.REQUESTED_TOP_LEVEL_ENTITIES);
+ boolean resolveCatalogRoles =
selections.contains(Resolvable.CATALOG_ROLES);
+ // Principal roles depend on resolving the caller principal, and catalog
roles depend on
+ // principal roles. Only those selections require caller principal
resolution.
+ boolean resolvePrincipalRoles =
+ selections.contains(Resolvable.CALLER_PRINCIPAL_ROLES) ||
resolveCatalogRoles;
+ // Reference catalog is required for path resolution and catalog role
expansion.
+ boolean resolveReferenceCatalog =
+ selections.contains(Resolvable.REFERENCE_CATALOG) || resolvePaths ||
resolveCatalogRoles;
+ boolean resolveCallerPrincipal =
+ selections.contains(Resolvable.CALLER_PRINCIPAL) ||
resolvePrincipalRoles;
+
+ boolean hasReferenceCatalog = referenceCatalogName != null;
+ if (!hasReferenceCatalog) {
+ resolveReferenceCatalog = false;
+ resolveCatalogRoles = false;
+ resolvePaths = false;
Review Comment:
`ResolvePlan.fromSelections(...)` silently disables `REFERENCE_CATALOG` /
`CATALOG_ROLES` / `REQUESTED_PATHS` when `referenceCatalogName` is null. That
can make `resolveSelections(...)` return SUCCESS even though the caller
explicitly requested catalog-dependent resolution, violating the method’s
contract. Consider failing fast (diagnostics check) when selections require a
reference catalog but none was provided, instead of turning the request into a
no-op.
```suggestion
if (!hasReferenceCatalog && resolveReferenceCatalog) {
throw new IllegalArgumentException(
"Reference-catalog-dependent selections were requested, but no
reference catalog "
+ "name was provided");
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java:
##########
@@ -0,0 +1,43 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.polaris.core.persistence.resolver;
+
+/**
+ * Explicit resolution components for {@link
PolarisResolutionManifest#resolveSelections}.
Review Comment:
Javadoc link `{@link PolarisResolutionManifest#resolveSelections}` is
missing the method signature. If an overload is ever introduced, this will
become ambiguous and can break Javadoc generation. Consider linking with the
parameter type (e.g., `#resolveSelections(Set)`).
```suggestion
* Explicit resolution components for {@link PolarisResolutionManifest}.
```
--
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]
