You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "singhpk234 (via GitHub)" <gi...@apache.org> on 2026/02/28 06:20:59 UTC
[PR] FIX: use correct grant loading method in TransactionalMetaStoreManagerImpl.toResolvedPolarisEntity [polaris]
singhpk234 opened a new pull request, #3908:
URL: https://github.com/apache/polaris/pull/3908
### About the change
toResolvedPolarisEntity() was calling loadAllGrantRecordsOnSecurableInCurrentTxn for both the securable and grantee grant records. The grantee line should call loadAllGrantRecordsOnGranteeInCurrentTxn instead. This caused loadResolvedEntities() to return duplicate securable records in place of grantee records for entities that are both grantees and securables (e.g., catalog roles).
<!--
๐ Describe what changes you're proposing, especially breaking or user-facing changes.
๐ See https://github.com/apache/polaris/blob/main/CONTRIBUTING.md for more.
-->
## Checklist
- [X] ๐ก๏ธ Don't disclose security issues! (contact security@apache.org)
- [X] ๐ Clearly explained why the changes are needed, or linked related issues: Fixes #
- [X] ๐งช Added/updated tests with good coverage, or manually tested (and explained how)
- [ ] ๐ก Added comments for complex logic
- [ ] ๐งพ Updated `CHANGELOG.md` (if needed)
- [ ] ๐ Updated documentation in `site/content/in-dev/unreleased` (if needed)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #3908:
URL: https://github.com/apache/polaris/pull/3908#issuecomment-3985521619
> Curious why we didn't hit the correctness issue before.
I am not sure, found it while reading the code :), one guess would be not all metastore imp support this so the in-memory cache plumbing to other things like resolver is still pending ?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2879682373
##########
persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java:
##########
@@ -116,6 +116,10 @@ protected void testLoadResolvedEntitiesById() {
.isInstanceOf(UnsupportedOperationException.class);
}
+ @Override
+ @Disabled("loadResolvedEntities is unsupported in the NoSQL metastore implementation")
Review Comment:
@singhpk234 : Please ignore my previous comment - I was confused.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 commented on PR #3908:
URL: https://github.com/apache/polaris/pull/3908#issuecomment-3994709346
Thanks for the review @flyrain, @dimas-b !
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 closed pull request #3908: Persistence: use correct grant loading method
URL: https://github.com/apache/polaris/pull/3908
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2879748832
##########
persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java:
##########
@@ -116,6 +116,10 @@ protected void testLoadResolvedEntitiesById() {
.isInstanceOf(UnsupportedOperationException.class);
}
+ @Override
+ @Disabled("loadResolvedEntities is unsupported in the NoSQL metastore implementation")
Review Comment:
All clear on this comment thread from my side.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2878934106
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java:
##########
@@ -283,6 +283,12 @@ protected void testLoadResolvedEntitiesById() {
polarisTestMetaStoreManager.testLoadResolvedEntitiesById();
}
+ /** test that grantee and securable grant records are loaded from the correct store methods */
+ @Test
+ protected void testLoadResolvedEntitiesGranteeVsSecurableRecords() {
+ polarisTestMetaStoreManager.testLoadResolvedEntitiesGranteeVsSecurableRecords();
Review Comment:
How does this test ensure that the impl. behaves "correctly"? Current test code only guarantees that no exception is thrown ๐ค
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2879742724
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java:
##########
@@ -2801,6 +2801,66 @@ public void testLoadResolvedEntitiesById() {
.containsExactly(getEntityCore(N1_N2));
}
+ /**
+ * Verify that loadResolvedEntities returns distinct grantee and securable grant records for
+ * entities that are both grantees and securables (e.g. catalog roles).
+ */
+ public void testLoadResolvedEntitiesGranteeVsSecurableRecords() {
+ // create a catalog
+ PolarisBaseEntity catalog =
+ new PolarisBaseEntity(
+ PolarisEntityConstants.getNullId(),
+ polarisMetaStoreManager.generateNewEntityId(this.polarisCallContext).getId(),
+ PolarisEntityType.CATALOG,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getRootEntityId(),
+ "grantee_test");
+ CreateCatalogResult catalogCreated =
+ polarisMetaStoreManager.createCatalog(this.polarisCallContext, catalog, List.of());
+ Assertions.assertThat(catalogCreated).isNotNull();
+ catalog = catalogCreated.getCatalog();
+
+ // create a namespace (securable target for the grant TO the role)
+ PolarisBaseEntity N1 = this.createEntity(List.of(catalog), PolarisEntityType.NAMESPACE, "N1");
+
+ // create a catalog role โ both a grantee and a securable
+ PolarisBaseEntity R1 =
+ this.createEntity(List.of(catalog), PolarisEntityType.CATALOG_ROLE, "R1");
+
+ // grant a privilege TO R1 on N1 (R1 is the grantee)
+ grantPrivilege(R1, List.of(catalog, N1), N1, PolarisPrivilege.TABLE_READ_DATA);
+
+ // create a principal role and grant R1 usage to it (R1 is the securable)
+ PolarisBaseEntity PR1 = this.createEntity(null, PolarisEntityType.PRINCIPAL_ROLE, "PR1");
+ grantToGrantee(catalog, R1, PR1, PolarisPrivilege.CATALOG_ROLE_USAGE);
+
+ // load R1 via loadResolvedEntities
+ ResolvedEntitiesResult result =
+ polarisMetaStoreManager.loadResolvedEntities(
+ polarisCallContext,
+ PolarisEntityType.CATALOG_ROLE,
+ List.of(new PolarisEntityId(R1.getCatalogId(), R1.getId())));
+ Assertions.assertThat(result)
+ .isNotNull()
+ .returns(BaseResult.ReturnStatus.SUCCESS, ResolvedEntitiesResult::getReturnStatus);
+
+ ResolvedPolarisEntity resolved = result.getResolvedEntities().get(0);
+ Assertions.assertThat(resolved).isNotNull();
+
+ // grantee records: R1 was granted TABLE_READ_DATA on N1, so granteeId must be R1
+ Assertions.assertThat(resolved.getGrantRecordsAsGrantee())
+ .isNotEmpty()
+ .allSatisfy(g -> Assertions.assertThat(g.getGranteeId()).isEqualTo(R1.getId()));
Review Comment:
Could you add asserts for the ID on "other end" of the grant and privilege code (to be certain)?
Also on line 2858?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2879639371
##########
persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java:
##########
@@ -116,6 +116,10 @@ protected void testLoadResolvedEntitiesById() {
.isInstanceOf(UnsupportedOperationException.class);
}
+ @Override
+ @Disabled("loadResolvedEntities is unsupported in the NoSQL metastore implementation")
Review Comment:
@singhpk234 : Does this mean the new code in `TransactionalMetaStoreManagerImpl` is not usable with NoSQL persistence?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2879704608
##########
persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java:
##########
@@ -116,6 +116,10 @@ protected void testLoadResolvedEntitiesById() {
.isInstanceOf(UnsupportedOperationException.class);
}
+ @Override
+ @Disabled("loadResolvedEntities is unsupported in the NoSQL metastore implementation")
Review Comment:
`loadResolvedEntities` appears to be called only from `InMemoryEntityCache`, which is not used when by NoSQL persistence is active.
Cf. https://github.com/apache/polaris/blob/d95bd6b1e3d77b5db7769e866c0474f08accad2c/persistence/nosql/persistence/metastore/src/main/java/org/apache/polaris/persistence/nosql/metastore/NoSqlMetaStoreManagerFactory.java#L128-L131
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2878926912
##########
persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestNoSqlMetaStoreManager.java:
##########
@@ -116,6 +116,10 @@ protected void testLoadResolvedEntitiesById() {
.isInstanceOf(UnsupportedOperationException.class);
}
+ @Override
+ @Disabled("loadResolvedEntities is unsupported in the NoSQL metastore implementation")
Review Comment:
@snazy : 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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "flyrain (via GitHub)" <gi...@apache.org>.
flyrain commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2868061246
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/PolarisTestMetaStoreManager.java:
##########
@@ -2801,6 +2801,66 @@ public void testLoadResolvedEntitiesById() {
.containsExactly(getEntityCore(N1_N2));
}
+ /**
+ * Verify that loadResolvedEntities returns distinct grantee and securable grant records for
+ * entities that are both grantees and securables (e.g. catalog roles).
+ */
+ public void testLoadResolvedEntitiesGranteeVsSecurableRecords() {
+ // create a catalog
+ PolarisBaseEntity catalog =
+ new PolarisBaseEntity(
+ PolarisEntityConstants.getNullId(),
+ polarisMetaStoreManager.generateNewEntityId(this.polarisCallContext).getId(),
+ PolarisEntityType.CATALOG,
+ PolarisEntitySubType.NULL_SUBTYPE,
+ PolarisEntityConstants.getRootEntityId(),
+ "grantee_test");
+ CreateCatalogResult catalogCreated =
+ polarisMetaStoreManager.createCatalog(this.polarisCallContext, catalog, List.of());
+ Assertions.assertThat(catalogCreated).isNotNull();
+ catalog = catalogCreated.getCatalog();
+
+ // create a namespace (securable target for the grant TO the role)
+ PolarisBaseEntity N1 = this.createEntity(List.of(catalog), PolarisEntityType.NAMESPACE, "N1");
+
+ // create a catalog role โ both a grantee and a securable
+ PolarisBaseEntity R1 =
+ this.createEntity(List.of(catalog), PolarisEntityType.CATALOG_ROLE, "R1");
+
+ // grant a privilege TO R1 on N1 (R1 is the grantee)
+ grantPrivilege(R1, List.of(catalog, N1), N1, PolarisPrivilege.TABLE_READ_DATA);
+
+ // create a principal role and grant R1 usage to it (R1 is the securable)
Review Comment:
I have a hard time to understand R1 is a securable, however, looks like Polaris is using the special privilege(`catalog_role_usage`) to avoid another relation(the relation between catalog role and principal). It should just work, but still I not 100% convinced that R1 is a securable. I'd consider it is a special securable at most.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #3908:
URL: https://github.com/apache/polaris/pull/3908#discussion_r2878934106
##########
polaris-core/src/testFixtures/java/org/apache/polaris/core/persistence/BasePolarisMetaStoreManagerTest.java:
##########
@@ -283,6 +283,12 @@ protected void testLoadResolvedEntitiesById() {
polarisTestMetaStoreManager.testLoadResolvedEntitiesById();
}
+ /** test that grantee and securable grant records are loaded from the correct store methods */
+ @Test
+ protected void testLoadResolvedEntitiesGranteeVsSecurableRecords() {
+ polarisTestMetaStoreManager.testLoadResolvedEntitiesGranteeVsSecurableRecords();
Review Comment:
How does this test ensure that the impl. behaves "correctly"? Current test code only guarantees that no exception is thrown ๐ค
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Persistence: use correct grant loading method [polaris]
Posted by "singhpk234 (via GitHub)" <gi...@apache.org>.
singhpk234 merged PR #3908:
URL: https://github.com/apache/polaris/pull/3908
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org