Remove unused private constructors from core

[dimas-b]

Remove some uncalled private constructors for the sake of clarity and ease of future maintenance.

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)

[flyrain]

cc @HonahX

[HonahX]

Hi @dimas-b Thanks for the effort of improving clarity of the core module. These private constructors are there for these classes to be JSON Serdes, which will be useful for downstream integration where the MetaStoreManager is talking to a remote persistence server via Rest API.

Could we keep these constructor to preserve the JSON Serdes for these classes. It would also be ok if we replace these with static JsonCreator if that help make things clearer.

I am also thinking of adding roundtrip test for these classes.

[dimas-b]

Thanks for the info, @HonahX !

However, from the OSS perspective this use case is absolutely not apparent 🤷 Adding tests can be a means for avoiding regressions in downstream, but the situation is still that a downstream project imposes a restriction on Polaris code in this case without any OSS use cases.

@flyrain : What is your take on this in light of the recent dev ML conversation?

I propose to use this PR as an illustration and continue the discussion of dev.

[HonahX]

Hi @dimas-b Thanks for sharing the thoughts!

IMHO this is not a requirement imposed by any downstream project. The JSON serde behavior has been in Polaris since the very beginning and has effectively become an existing convention in the core module. We can still see many similar annotations across the codebase today:

• PolarisEntityId
• PolarisEntitySubType
• ...
  In that sense, removing these private @JsonCreator constructors is not just a cleanup — it changes the long-standing implication that these classes are expected to be JSON-serializable. Hence, I feel the situation is different from the dev ML conversion linked above

Regarding potential use cases: the downstream integration example I mentioned was just one illustration. Other scenarios, such as migrating or syncing principals or entities between Polaris deployments, could also reasonably rely on these classes being JSON serde.

In general, I would prefer that we not lose this established assumption and flexibility purely for the sake of having less code. If the private constructors themselves can be considered confusing, we could replace them with static json creator, and switching to builder/Immutable for result classes for example.

Please let me know WDYT!

[dimas-b]

@HonahX : How can the serde removed in this PR be a convention in Polaris, when it is not used in Polaris build or runtime?

My exact point is that it is a convention in a downstream project, but not in Polaris itself.

As I mentioned above, I do not insist on merging this PR. However, it is an illustration of downstream dependencies that exists only in downstream code. Successful CI on this PR shows that Polaris itself does not rely on this code.

Ideally, IMHO, downstream projects that rely on serializing these objects should probably implement and maintain serde (via JSON or overwise) by themselves.

[HonahX]

Hi @dimas-b. The way I see it, even though the JSON-serde paths aren’t exercised in Polaris today, the @JsonCreator constructors have been there for long time acting as an implicit signal that these classes are intended to round-trip through JSON. In other words, the annotations themselves set an expectation for how future changes to these classes should behave, regardless of whether the current runtime calls into that path.

Whether we actually want to keep JSON serde as part of the core module’s design in the future could be a separate discussion. It seems we may have different views on that, which is totally fine — I think it’s something worth discussing with the community so we can align on the long-term direction together. I was about to raise a PR to restore JSON serde for PolarisEntityCore (which was removed as a side effect of #1596), but that should depend on the direction we decide collectively.