[Core, Bug] CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted. by HonahX · Pull Request #3219 · apache/polaris

HonahX · 2025-12-05T17:49:52Z

In TransactionalMetaStoreManagerImpl.createEntitiesIfNotExist, we currently directly return the entity received in the arg if the entity does not exist:

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/transactional/TransactionalMetaStoreManagerImpl.java

Lines 1087 to 1091 in be3c88b

    
           // persist that new entity 
        
           this.persistNewEntity(callCtx, ms, entity); 
        
           // done, return that newly created entity 
        
           return new EntityResult(entity);

However, the persistNewEntity above will update some field including lastUpdatedTimestamp before writing the entity

polaris/polaris-core/src/main/java/org/apache/polaris/core/persistence/BaseMetaStoreManager.java

Lines 108 to 111 in be3c88b

    
           entityBuilder.lastUpdateTimestamp(entity.getCreateTimestamp()); 
        
           entityBuilder.dropTimestamp(0); 
        
           entityBuilder.purgeTimestamp(0); 
        
           entityBuilder.toPurgeTimestamp(0);

As a result, the entity included in the EntityResult is not the same as the actual entity persisted.

The similar issue also happened in CreatePrincipal for both atomic and transactional metastore manager.

How to reproduce

PolarisMetaStoreManager metaStoreManager = polarisTestMetaStoreManager.polarisMetaStoreManager;
    PolarisCallContext callCtx = polarisTestMetaStoreManager.polarisCallContext;
    PolarisBaseEntity principalEntity =
        metaStoreManager
            .createPrincipal(
                callCtx,
                new PrincipalEntity.Builder()
                    .setId(metaStoreManager.generateNewEntityId(callCtx).getId())
                    .setName("principal_test")
                    .setCreateTimestamp(100L)
                    .build())
            .getPrincipal();

    PolarisBaseEntity fetchedPrincipal =
        metaStoreManager
            .readEntityByName(
                callCtx,
                null,
                PolarisEntityType.PRINCIPAL,
                PolarisEntitySubType.NULL_SUBTYPE,
                "principal_test")
            .getEntity();
     
    // The assertion will fail because the principalEntity have lastUpdatedTimestamp = 0 while fetchedPrincipal have lastUpdatedTimestamp = 100L
    Assertions.assertThat(principalEntity).isEqualTo(fetchedPrincipal);

The principalEntity returned from createPrincipal will have lastUpdatedTimestamp equals 0 but the fetchedEntity will have that equals 100. (the actual version persisted)

Fix

The PR fixes the issue by letting persistEntity return the entity persisted and include that in the EntityResult. The PR also include new unit tests to verify the behavior

Checklist

🛡️ Don't disclose security issues! (contact [email protected])
🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
🧪 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)

dimas-b · 2025-12-06T00:58:52Z

CHANGELOG.md

 - Added checksum to helm deployment so that it will restart when the configmap has changed.
 - Generic Table is no longer in beta and is generally-available.
 - Added Windows support for Python client
+- Ensure createEntitiesIfNotExist and createPrincipal return the entity actually persisted.


Is the fix is specific to TransactionalMetaStoreManagerImpl or applies to all implementations? (my impression is the former, but this message is generic) 🤔

IMHO, this fix does not affect Polaris end users, so it does not have to be mentioned in CHANGELOG (but it's ok to mention).

If the effects of this fix are visible in REST APIs, I'd propose to reformulate the change log entry in terms of REST API.

It affects CreateEntitiesIfNotExist in TransactionalMetaStoreManagerImpl and createPrincipal in all implementations. But yeah, this should not affect any user-facing feature/experience now. Removed the CHANGELOG line

Thx, @HonahX !

As for TransactionalMetaStoreManagerImpl, I believe it is not in the JDBC persistence call paths, so I'll defer to @dennishuo for actual code review 🙂

singhpk234

LGTM too, thanks @HonahX for the fix !

adnanhemani

Nice catch @HonahX! LGTM but similar question as @dimas-b's comment :)

dimas-b

Changes in AtomicOperationMetaStoreManager and tests LGTM 👍

I'll defer to @dennishuo for TransactionalMetaStoreManagerImpl review.

dennishuo

LGTM, good find! Looks like this was perhaps a regression actually inadvertently introduced in 9ff2ca1#diff-4518e9ca1448a8d56b5ddf35523289a6f70a9d2c1bfb91c62105b6199bcb8788 where prepareToPersistNewEntity in BasePolarisMetaStoreManager was changed from

// this is the first change
// TODO: Make immutable; make sure no caller depends on the input entity actually
// being changed.
entity.setLastUpdateTimestamp(entity.getCreateTimestamp());

// set all other timestamps to 0
entity.setDropTimestamp(0);
entity.setPurgeTimestamp(0);
entity.setToPurgeTimestamp(0);
return entity;

to

 PolarisBaseEntity.Builder entityBuilder = new PolarisBaseEntity.Builder(entity);
  entityBuilder.lastUpdateTimestamp(entity.getCreateTimestamp());
  entityBuilder.dropTimestamp(0);
  entityBuilder.purgeTimestamp(0);
  entityBuilder.toPurgeTimestamp(0);
  return entityBuilder.build();

I think the TODO was trying to warn against exactly this situation, but it can sometimes be hard to tell when an accidental dependency on the entity being changed sneaks in somewhere. We might want to do another check based on all the warning-TODOs removed there to make sure nothing else is subtly broken (I guess the main manifestation would just be that response payloads may be missing some fields that callers might not have been using if they haven't complained).

HonahX · 2025-12-08T23:52:04Z

Thanks @dimas-b @singhpk234 @adnanhemani @dennishuo for reviewing! Merging

* Doc cleanup for local deployment (apache#3213) * Doc cleanup for admin tool (apache#3214) * Bump version from 1.0.0 to 1.2.0 and fix health port (apache#3211) * fix(deps): update dependency io.opentelemetry:opentelemetry-bom to v1.57.0 (apache#3223) * fix(deps): update dependency org.apache.commons:commons-text to v1.15.0 (apache#3233) * fix(deps): update dependency software.amazon.awssdk:bom to v2.40.3 (apache#3234) * Core: Add timeout and retry logic to Azure token fetch (apache#3113) * update markdown lint check (apache#3187) use tcort/github-action-markdown-link-check, gaurav-nelson/github-action-markdown-link-check is deprecated * NoSQL: Add metastore types and mappings (apache#3207) Add the NoSQL specific metastore persistence types including the mapping from and to `*Polaris*Entity`. * NoSQL/nit: fix javadoc for `Realms` (apache#3229) * Fix build issue for docker not found when using latest docker desktop (apache#3227) * fix(deps): update dependency org.mongodb:mongodb-driver-sync to v5.6.2 (apache#3238) * fix(deps): update immutables to v2.12.0 (apache#3240) * fix(deps): update dependency io.micrometer:micrometer-bom to v1.16.1 (apache#3239) * [Core, Bug] CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted. (apache#3219) The PR fixes the issue, "CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted", by letting persistEntity return the entity persisted and include that in the EntityResult. The PR also include new unit tests to verify the behavior * (feat) doc: Update Makefile to fix admonitions in helm doc and remove redundant sections (apache#3232) * Change org.testcontainers:<dep> to org.testcontainers:testcontainers-<dep> (apache#3225) * Helm: add support for topologySpreadConstraints (apache#3216) * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.23-6.1764764731 (apache#3241) * NoSQL correctness tests: add missing `logback-test.xml` files (apache#3230) * Add Docker-based Ceph + Polaris cluster setup (apache#3022) --------- Co-authored-by: sarunas.svegzda <[email protected]> * Service: Remove *CommitTableEvent, Add *UpdateTableEvent to Transactions (apache#3195) * Update dependency pydantic to >=2.12.5,<2.13.0 (apache#2807) * fix(deps): update dependency com.github.dasniko:testcontainers-keycloak to v4.0.1 (apache#3244) * fix(deps): update mockito monorepo to v5.21.0 (apache#3245) * Allow retrieving a config directly from a `Map` (apache#3220) The current implementation deserializes the catalog configuration properties for each invocation of `getConfig*()` taking a `CatalogEntity`. This change adds another `getConfig*()` variant that takes a `Map` to allow call sites to memoize the properties, where possible. * Runtime/service: move getConfig() down to `IcebergCatalogHandler` (apache#3231) All catalog specific functionality is implemented in `IcebergCatalogHandler`, whereas `IcebergCatalogAdapter` is meant to act as a "REST wrapper" to it. This change moves the implementation of `getConfig` down to the handler, no functional changes. * chore(deps): update quay.io/ceph/ceph docker tag to v20 (apache#3242) * fix(deps): update quarkus platform and group to v3.30.3 (apache#3247) * NoSQL: Prepare for NoSQL tests (apache#3235) * Add an optional `bootstrapRealm()` implementation to `PolarisAuthzTestBase` * Allow extending `IcebergCatalogHandlerAuthzTest`, move tests to `AbstractIcebergCatalogHandlerAuthzTest` * No functional changes * Shell script to verify staged release candidate artifacts (apache#2824) Performs a bunch of verifications against a proposed (staged) release candidate using the new `tools/verify-release/verify-release.sh` script against Maven artifacts, main distributions and Helm chart. Checks: * GPG signature and checksum verifications * All expected artifacts are present * Build artifacts are reproducible (minus known exceptions) * jar files * Main distribution zip/tarball * Helm chart * Build passes. * DISCLAIMER/LICENSE/NOTICE files are present in artifacts that require those More information in the added web site page. Fixes apache#2822 --------- Co-authored-by: Pierre Laporte <[email protected]> * Core: Add GCP service account impersonation for credentials. (apache#3246) * fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.22 (apache#3253) * fix(deps): update dependency com.google.cloud:google-cloud-iamcredentials to v2.80.0 (apache#3254) * feat: pass principal name as part of aws subscoped credentials session (apache#3224) * feat: pass principal name as part of aws subscoped credentials session name * feat: resolve principal from CurrentIdentityAssociation * fix: handle principal injection for async tasks * add feature flag for principal name include * add changelog, address comments * handle null identity, refactor tests * Added user token to the PolarisPrincipal (apache#3236) * Added user token to the PolarisPrincipal * added redacted * Fix compilation failures in GcpCredentialsStorageIntegrationTest (apache#3257) * chore(deps): update github artifact actions (apache#3260) * chore(deps): update medyagh/setup-minikube action to v0.0.21 (apache#3264) * NoSQL: Metastore implementation (apache#3237) * Fix typo in nosql (apache#3263) * Corrected a typo in a key configuration parameter in the 1.2.0 release notes (apache#3262) * fix(deps): update dependency software.amazon.awssdk:bom to v2.40.8 (apache#3271) * Add NOTES.txt to Helm chart with installation instructions (apache#3173) * Add NOTES.txt to Helm chart with installation instructions Provides port-forward commands, health check endpoint, and log viewing for users after installation. * Fix helm unittest for GH action (apache#3279) * [doc]: Doc fix for CLI usage (apache#3215) * [doc]: Add doc for helm prod deployment (apache#3265) * chore(deps): update docker.io/prom/prometheus docker tag to v3.8.1 (apache#3282) * chore(deps): update dependency jupyterlab to v4.5.1 (apache#3275) * fix(deps): update dependency com.google.cloud:google-cloud-storage-bom to v2.61.0 (apache#3274) * chore(deps): update dependency mypy to >=1.19, <=1.19.1 (apache#3272) * Bump to 1.4.0-incubating-SNAPSHOT (apache#3181) * Bump to 1.4.0-incubating-SNAPSHOT * Update Python client version * Add exclude check note in the release guide (apache#3182) * Add exclude check note in the release guide * Update site/content/release-guide.md Co-authored-by: Robert Stupp <[email protected]> --------- Co-authored-by: Robert Stupp <[email protected]> * docs(tools): Create the Tools Section in the Docs (apache#3189) * fix(deps): update dependency org.apache.logging.log4j:log4j-core to v2.25.3 (apache#3283) * Add Polaris Community Meeting 20251211 (apache#3284) * chore(deps): update dependency pre-commit to v4.5.1 (apache#3286) * fix(deps): update dependency com.google.cloud:google-cloud-iamcredentials to v2.81.0 (apache#3287) * ensure AddressResolver supports localhost even if ipv6 is disabled in sysctl but not /etc/hosts (apache#3285) * Migrate to Jackson mapper builder pattern (apache#3269) Mappers and factories are fully immutable objects in Jackson 3. This change is rather a no-op, but migrates the code to use the builder-pattern. This is only a little building-block for "real" Jackson 3 support, there's more to do and more that's required from other frameworks. * fix(deps): update quarkus platform and group to v3.30.4 (apache#3291) * Rework release guide to include workflows (apache#3273) * Add a release guides section * Rename current release guide to manual (deprecated) * Add new semi-automated release guide * Move release verification guide under release guides section * Add scss style for better screenshot separation * Add redirection from old pages to new ones Co-authored-by: Robert Stupp <[email protected]> * Site: Fix typos in release guide (apache#3296) * [chore]: Match openapi-generator-cli version in build system to dependency (apache#3266) * Fix openapi-generator-cli version in build system * Fix openapi-generator-cli version in build system * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.24-1 (apache#3297) * chore(deps): update dependency openapi-generator-cli to v7.17.0 (apache#3298) * chore(deps): update docker.io/mongo docker tag to v8.2.3 (apache#3299) * chore(deps): update mongo docker tag to v8.2.3 (apache#3300) * fix(deps): update dependency io.smallrye.config:smallrye-config-core to v3.15.0 (apache#3302) * fix(deps): update dependency org.apache.httpcomponents.client5:httpclient5 to v5.6 (apache#3301) * chore(deps): update plugin com.gradle.develocity to v4.3 (apache#3248) * Unify mongo image ref (apache#3303) To prevent duplicate version-bump PRs like apache#3299 and apache#3300 * fix(deps): update dependency org.testcontainers:testcontainers-bom to v2.0.3 (apache#3277) * Disable sectionPagesMenu (apache#3312) * Remove docker-java.properties (apache#3307) * Ensure release can only run from specific SHA (apache#3295) * Ensure release publish workflow can only run from last RC (apache#3290) * Enable use of second release workflow for RC>0 * Patch 3rd workflow to support commits with multiple RC tags * Force 4th workflow to only run from a release branch * Update release guide to match new workflows * fix(deps): update dependency ch.qos.logback:logback-classic to v1.5.23 (apache#3308) * fix(deps): update dependency software.amazon.awssdk:bom to v2.40.13 (apache#3309) * chore(deps): update registry.access.redhat.com/ubi9/openjdk-21-runtime docker tag to v1.24-2 (apache#3313) * NoSQL: reduce heap pressure when running tests Some tests generate a lot of realms, likely one realm per test case. While the amount of data per realm is not much, it is nontheless nice to remove that data immediately (for tests). The maintenance service, which purges data of eligible realms, cannot be run against the in-memory backend (different JVM). This change adds a rather "test only" workaround to purge the realm data in the in-memory backend immediately. * NoSQL: Metastore maintenance Implementation of the NoSQL meta-store maintenance implementation. It adds the meta-store specific handling to the existing NoSQL maintenance service to purge unreferenced and unneeded data from the database. * NoSQL: Add to runtime-service * NoSQL: Add metastore-maintenance to admin tool * NoSQL: revert LICENSE file change * Last merged commit 62d774f --------- Co-authored-by: Yong Zheng <[email protected]> Co-authored-by: Mend Renovate <[email protected]> Co-authored-by: fivetran-rahulprakash <[email protected]> Co-authored-by: Kevin Liu <[email protected]> Co-authored-by: Honah (Jonas) J. <[email protected]> Co-authored-by: Šarūnas Švėgžda <[email protected]> Co-authored-by: sarunas.svegzda <[email protected]> Co-authored-by: Adnan Hemani <[email protected]> Co-authored-by: Pierre Laporte <[email protected]> Co-authored-by: Talat UYARER <[email protected]> Co-authored-by: Tornike Gurgenidze <[email protected]> Co-authored-by: cccs-cat001 <[email protected]> Co-authored-by: Alexandre Dutra <[email protected]> Co-authored-by: zgxme <[email protected]> Co-authored-by: Tamas Mate <[email protected]> Co-authored-by: JB Onofré <[email protected]> Co-authored-by: Adam Christian <[email protected]> Co-authored-by: Romain Manni-Bucau <[email protected]>

Fix createEntityIfNotExists in TransactionalMetaStoreManagerImpl

cbb1262

github-project-automation bot added this to Basic Kanban Board Dec 5, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board Dec 5, 2025

Fix AtomicOperationMetaStoreManager similar issue

3149c6c

HonahX changed the title ~~[Core, Bug] TransactionalMetaStoreManager.createEntitiesIfNotExist not return the same entity persisted.~~ [Core, Bug] CreateEntitiesIfNotExist not return the same entity persisted. Dec 5, 2025

HonahX changed the title ~~[Core, Bug] CreateEntitiesIfNotExist not return the same entity persisted.~~ [Core, Bug] CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted. Dec 5, 2025

Add change log

74dada8

HonahX marked this pull request as ready for review December 5, 2025 23:19

HonahX requested a review from dennishuo December 5, 2025 23:20

dimas-b previously approved these changes Dec 6, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Dec 6, 2025

singhpk234 previously approved these changes Dec 6, 2025

View reviewed changes

adnanhemani approved these changes Dec 8, 2025

View reviewed changes

Remove changelog

3878a47

HonahX dismissed stale reviews from singhpk234 and dimas-b via 3878a47 December 8, 2025 18:38

HonahX requested review from adnanhemani, dimas-b and singhpk234 December 8, 2025 18:39

dimas-b reviewed Dec 8, 2025

View reviewed changes

dennishuo approved these changes Dec 8, 2025

View reviewed changes

HonahX merged commit 0d4e0ff into apache:main Dec 8, 2025
15 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Dec 8, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core, Bug] CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted.#3219

[Core, Bug] CreateEntitiesIfNotExist/CreatePrincipal not return the same entity persisted.#3219
HonahX merged 4 commits intoapache:mainfrom
HonahX:honahx-fix-create-entity-issue

HonahX commented Dec 5, 2025 •

edited

Loading

Uh oh!

dimas-b Dec 6, 2025

Uh oh!

dimas-b Dec 6, 2025

Uh oh!

dimas-b Dec 6, 2025

Uh oh!

HonahX Dec 8, 2025 •

edited

Loading

Uh oh!

dimas-b Dec 8, 2025

Uh oh!

singhpk234 left a comment

Uh oh!

adnanhemani left a comment

Uh oh!

dimas-b left a comment •

edited

Loading

Uh oh!

dennishuo left a comment

Uh oh!

HonahX commented Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// persist that new entity
	this.persistNewEntity(callCtx, ms, entity);

	// done, return that newly created entity
	return new EntityResult(entity);

	entityBuilder.lastUpdateTimestamp(entity.getCreateTimestamp());
	entityBuilder.dropTimestamp(0);
	entityBuilder.purgeTimestamp(0);
	entityBuilder.toPurgeTimestamp(0);

Conversation

HonahX commented Dec 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to reproduce

Fix

Checklist

Uh oh!

dimas-b Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b Dec 6, 2025

Choose a reason for hiding this comment

Uh oh!

HonahX Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dimas-b Dec 8, 2025

Choose a reason for hiding this comment

Uh oh!

singhpk234 left a comment

Choose a reason for hiding this comment

Uh oh!

adnanhemani left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennishuo left a comment

Choose a reason for hiding this comment

Uh oh!

HonahX commented Dec 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

HonahX commented Dec 5, 2025 •

edited

Loading

HonahX Dec 8, 2025 •

edited

Loading

dimas-b left a comment •

edited

Loading