XN137 commented on code in PR #2518:
URL: https://github.com/apache/polaris/pull/2518#discussion_r2391117261
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/dao/entity/CreatePrincipalResult.java:
##########
@@ -67,10 +71,22 @@ private CreatePrincipalResult(
super(returnStatus, extraInformation);
this.principal = principal;
this.principalSecrets = principalSecrets;
+ validate();
Review Comment:
i am in a similar boat as Dmitri as i dont immediately see how/where the
JSON de-serialization is actually getting used.
i also kept the `principal` field as a `PolarisBaseEntity` so that we dont
hard-break the ser/deser logic of the class.
but for me the `validate()` method enforced the "invariant" of this class
(i.e. what is a valid `CreatePrincipalResult` instance).
correct me if i am wrong but afaict this has not really changed since the
original addition of the class:
https://github.com/apache/polaris/blame/4b24da5ef9e3e6093fc8f17cefeefceba11e1783/polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisMetaStoreManager.java#L289-L340
so in that sense we should be fine about backwards-compatibility.
if a system uses the json deserialization, wouldnt it want to validate that
the given input translates to a valid `CreatePrincipalResult` instance?
whats the benefit of being able to create an "invalid"
`CreatePrincipalResult` instance?
imo such a system should have explicit error handling about its
de-serialization usage (i.e. wrap instance creation in a try-catch?)
--
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]
