-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add validation for immutable fields #180
Conversation
@@ -27,6 +41,12 @@ func TestCopyFields(t *testing.T) { | |||
}) | |||
} | |||
|
|||
func TestValidateRequiredFields(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved this down from higher in file to have test cases in alpha order 😅
cfe8a76
to
a2f4b39
Compare
fieldbehavior/fieldbehavior_test.go
Outdated
CreateTime: timestamppb.Now(), // has OUTPUT_ONLY field_behavior; should be cleared. | ||
DisplayName: "site one", // has REQUIRED field_behavior; should not be cleared. | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO one t.Run should be readable as one paragraph, i.e. blank lines between t.Runs but not within
fieldbehavior/immutable.go
Outdated
return Has(field, annotations.FieldBehavior_IMMUTABLE) | ||
} | ||
|
||
func isSetOnWire(m protoreflect.Message, field protoreflect.FieldDescriptor) bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this just an alias for protoreflect.Message.Has? IMO better to stick to the original API even if the naming is not perfect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I added it make the conditional easier to skim read but am ok to revert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - apart from stylistic comments about preferring blank lines between functions but not within
a2f4b39
to
1ebc6cb
Compare
Within the Field Behavior AIP there is guidance on how to handle the
IMMUTABLE
field behavior, suggesting that:This PR introduces a new validation function
fieldbehavior.ValidateImmutableFieldsWithMask
to help service owners implement this suggestion and can return anINVALID_ARGUEMNT
if it makes sense for their service.It is heavily influenced by
fieldbehavior.ValidateImmutableFieldsWithMask
and copies some of that code but I felt it was more readable to copy and paste rather than having a generic implementation for both uses.As the recommendations are should and not must I didn't feel like this library should be too opinionated and error on the
fieldmask.Update
function.I also updated some of the comments and added a simple test for
fieldbehavior.ClearFields()
.