HDDS-13964. Validate objectID/updateID in WithObjectID.build() #9356
Conversation
swamirishi
left a comment
swamirishi
left a comment
There was a problem hiding this comment.
@adoroszlai thanks for the patch I have a few suggestions here design wise. Check and see if it makes sense
| } | ||
|
|
||
| public OmMultipartKeyInfo build() { | ||
| validate(); |
There was a problem hiding this comment.
Instead of each build method calling validate we can call super.build() and that could call validate() method. The changes wouldn't be this big.
There was a problem hiding this comment.
I feel this would be error prone again for newer implementations which could miss this.
There was a problem hiding this comment.
Instead of each build method calling validate we can call super.build() and that could call validate() method. The changes wouldn't be this big.
super.build() returns WithObjectID, not the specific subtype. If we need to override to cast the return value, there is no guarantee the implementation calls either super.build() or validate(). Maybe we can add a unit test for that.
I feel this would be error prone again for newer implementations which could miss this.
There are much fewer Builder subclasses than users of Builder.
There was a problem hiding this comment.
The buildObject would be the previous build() method implementation or the current buildMaybeInvalid() and actual build() object will never get overridden maybe we can make it final
There was a problem hiding this comment.
Added generic type to WithObjectID.Builder for this to work.
| } | ||
|
|
||
| protected Builder(WithObjectID obj) { | ||
| super(obj); |
There was a problem hiding this comment.
nit :
The parameter type here should be WithMetadata
There was a problem hiding this comment.
Well spotted, but this was not introduced in this PR (WithMetadata is not changed at all).
There was a problem hiding this comment.
yeah I know was just wondering if we could fix this along with this patch
| } | ||
| } | ||
|
|
||
| protected abstract WithObjectID buildMaybeInvalid(); |
There was a problem hiding this comment.
| protected abstract WithObjectID buildMaybeInvalid(); | |
| protected abstract WithObjectID buildObject(); | |
| public final WithObjectID build() { | |
| validate(); | |
| return buildObject(); | |
| } |
swamirishi
left a comment
swamirishi
left a comment
There was a problem hiding this comment.
LGTM thanks for the patch @adoroszlai
|
Thanks @swamirishi for the review. |
2 participants
What changes were proposed in this pull request?
WithObjectID.Builderand its subclasses currently provide two similar methods for settingobjectIDandupdateID:with...ID: validate inputset...ID: no validationThis PR moves validation from
with...IDtobuild()(or rathervalidate(), called bybuild()). That is where other properties (in subclasses) are validated, and is common practice for builders.Benefits:
Builderdo not need to remember to usewith...for validation, thus we can avoid potential bugs.getObjectInfo(), used by validation ofupdateID, did not provide any information about the object since builder does not customisetoString(). If validation fails, the message can now reuse a temporary instance of the value object (obtained viabuildMaybeInvalid()).Also make
WithParentObjectIdabstract.https://issues.apache.org/jira/browse/HDDS-13964
How was this patch tested?
CI:
https://github.com/adoroszlai/ozone/actions/runs/19629749773