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

[HonahX]

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]

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

[dimas-b]

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).

[dimas-b]

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.

[HonahX]

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

[dimas-b]

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]

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