-
Notifications
You must be signed in to change notification settings - Fork 140
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
Add validation for enum cases during contract updates #762
Conversation
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.
Thank you for adding this @SupunS!
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.
Looks good! just a couple minor suggestions
runtime/errors.go
Outdated
declName string | ||
expected int | ||
found int |
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.
All other errors export the fields so maybe do the same here
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.
Added in 51bb585
fmt.Sprintf( | ||
"mismatching enum case: expected `%s`, found `%s`", | ||
expectedEnumCase, | ||
foundEnumCase, | ||
), | ||
enumMismatchError.Error(), |
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.
Maybe instead of comparing error messages literally, just check declName, expectedEnumCase, and foundEnumCase of the error directly, so changing the error message doesn't break the test
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.
Also wanted to assert the error message, to make sure they are consistent, and the to-string works with nested errors, etc.
assert.Equal( | ||
t, | ||
fmt.Sprintf( | ||
"missing cases in enum `%s`: expected %d or more, found %d", | ||
declName, | ||
expectedCaes, | ||
foundCases, | ||
), | ||
extraFieldError.Error(), | ||
) |
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.
Maybe instead of comparing error messages literally, just check declName, expectedCases, and foundCases of the error directly, so changing the error message doesn't break the test
b938b28
to
51bb585
Compare
Closes #757
Description
As in #757, the enum-case values are represented using a composite value, having a field called
rawValue
which stores the enum-case raw value. Hence serializing/deserializing, only the raw value gets serialized/deserialized, but not the actual enum-case name. So changing enum-cases can lead to type/value confusions.This PR adds a validation to avoid such scenarios. With these changes, update to an enum need to make sure that:
The above restrictions would mean:
For contributor use:
master
branchFiles changed
in the Github PR explorer