Copilot commented on code in PR #3916:
URL: https://github.com/apache/polaris/pull/3916#discussion_r2869658160
##########
polaris-core/src/test/java/org/apache/polaris/core/persistence/ResolverTest.java:
##########
@@ -129,11 +129,28 @@ public void
testResolveSelectionsRequiresCallerPrincipalForCatalogRoles() {
PolarisPrincipal.of("missing", Map.of(), Set.of()),
null,
"test");
- ResolverStatus status =
resolver.resolveSelections(Set.of(Resolvable.CATALOG_ROLES));
+ ResolverStatus status =
resolver.resolveSelections(Set.of(Resolvable.CALLER_CATALOG_ROLES));
Assertions.assertThat(status.getStatus())
.isEqualTo(ResolverStatus.StatusEnum.CALLER_PRINCIPAL_DOES_NOT_EXIST);
}
+ @Test
+ public void
+
testResolveSelectionsRequestedTopLevelEntitiesWithCatalogRoleResolvesReferenceCatalog()
{
+ Resolver resolver =
+ new Resolver(
+ diagServices,
+ callCtx(),
+ metaStoreManager(),
+ PolarisPrincipal.of("missing", Map.of(), Set.of()),
+ null,
+ "test");
+ resolver.addOptionalEntityByName(PolarisEntityType.CATALOG_ROLE, "role1");
+ ResolverStatus status =
+
resolver.resolveSelections(Set.of(Resolvable.REQUESTED_TOP_LEVEL_ENTITIES));
+
Assertions.assertThat(status.getStatus()).isEqualTo(ResolverStatus.StatusEnum.SUCCESS);
Review Comment:
This new test name claims the reference catalog is resolved, but the
assertions only check SUCCESS. Consider also asserting that
getResolvedReferenceCatalog() is non-null (and/or has the expected name) to
ensure the test is actually validating the intended behavior rather than just
avoiding an exception.
```suggestion
Assertions.assertThat(status.getStatus()).isEqualTo(ResolverStatus.StatusEnum.SUCCESS);
Assertions.assertThat(resolver.getResolvedReferenceCatalog()).isNotNull();
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java:
##########
@@ -29,8 +29,8 @@ public enum Resolvable {
CALLER_PRINCIPAL,
/** Resolve the caller's activated principal-role entities. */
CALLER_PRINCIPAL_ROLES,
- /** Resolve catalog-role entities (e.g., roles attached in the reference
catalog). */
- CATALOG_ROLES,
+ /** Resolve caller-activated catalog-role entities in the reference catalog.
*/
+ CALLER_CATALOG_ROLES,
Review Comment:
The PR title says "rename Resolvable.CALLER_CATALOG_ROLES", but the code
change is actually renaming the old Resolvable.CATALOG_ROLES to
CALLER_CATALOG_ROLES. Consider updating the PR title to avoid confusion when
scanning history/blame.
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolvable.java:
##########
@@ -39,6 +39,11 @@ public enum Resolvable {
* Resolve any additional top-level entities explicitly registered via
addTopLevelName, such as
* catalog/principal/principal-role names used for authorization beyond the
caller and reference
* catalog.
+ *
+ * <p>Note: this currently also covers requested {@code CATALOG_ROLE} names
because they share the
+ * same {@link PolarisResolutionManifest} registration surface ({@code
addTopLevelName}). Although
+ * grouped here for compatibility, catalog-role name resolution still
requires the reference
+ * catalog context.
Review Comment:
The new note says REQUESTED_TOP_LEVEL_ENTITIES also covers requested
CATALOG_ROLE names via PolarisResolutionManifest.addTopLevelName, but
PolarisResolutionManifest’s Javadoc currently documents addTopLevelName as only
for (Catalog, Principal, PrincipalRole). Please align the documentation (either
broaden the manifest Javadoc or adjust this note) so callers aren’t misled
about what entity types are supported.
```suggestion
* Resolve any additional top-level entities explicitly registered via
* {@link PolarisResolutionManifest#addTopLevelName}, such as
catalog/principal/principal-role
* names used for authorization beyond the caller and reference catalog.
*
* <p>All entity types resolved here share the same
* {@link PolarisResolutionManifest#addTopLevelName} registration surface.
```
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/resolver/Resolver.java:
##########
@@ -439,18 +441,19 @@ private ResolverStatus runResolvePass(ResolvePlan plan) {
this.diagnostics.checkNotNull(this.referenceCatalogName,
"reference_catalog_expected");
status =
this.resolveReferenceCatalog(
- toValidate, this.referenceCatalogName,
plan.resolveCatalogRoles());
+ toValidate, this.referenceCatalogName,
plan.resolveCallerCatalogRoles());
}
Review Comment:
Naming is now inconsistent: ResolvePlan uses resolveCallerCatalogRoles(),
but resolveReferenceCatalog(...) still takes a boolean named
resolveCatalogRoles and its Javadoc talks about "catalog roles" generically. To
keep the rename unambiguous, consider renaming the parameter/Javadoc to
explicitly say caller-activated catalog roles.
--
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]
