Allow structs to be self-verifying. #293
Conversation
nolag
force-pushed
the
main
branch
3 times, most recently
from
dfa67ca to
b5d4107
Compare
|
hey @nolag , thanks for this PR! I'll try to review it asap. In the meantime can you rebase it and resolve the conflicts? thank you! |
Done, I rebased the PR. |
There was a problem hiding this comment.
hey @nolag, a few points here:
- the generated code for enums in the tests seem to lack a
vvariable - was it intended not to check for
requiredin the verify functions? - I think this flag should be orthogonal to
--only-models, or maybe we should rethink the two features so that they can work well with one another. at the moment--struct-verifyis ignored if--only-modelsis passed - I would like to have a suite of integration tests (eg:
verify_test.go) that run on the Verify functions of the generated code, similarly to what's been done intests/unmarshal_test.go: that is because just verifying that the generated code is what we expect is not sufficient, we should also verify it is valid and that it behaves properly
Thank you!
| func TestValidation(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| testExamples(t, basicConfig, "./data/validation") | ||
| cfg := basicConfig | ||
| cfg.StructVerify = true | ||
|
|
||
| testExamples(t, cfg, "./data/validation") | ||
| } |
There was a problem hiding this comment.
I would prefer having two suites of tests, one with the classic validation and one with the struct verify. wdyt?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyBooleanTypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyBooleanTypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
There was a problem hiding this comment.
interesting, that means that the compiled enum.go file wasn't ever validated before.
There was a problem hiding this comment.
correct. I started slowly adding that kind of coverage when I took ownership of maintaining this library, but it'll take some time.
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyBooleanUntypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyBooleanUntypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyIntegerTypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyIntegerTypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyNumberTypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyNumberTypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyNumberUntypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyNumberUntypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyStringTypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyStringTypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
| // Verify checks all fields on the struct match the schema. | ||
| func (plain EnumMyStringUntypedEnum) Verify() error { | ||
| for _, expected := range enumValues_EnumMyStringUntypedEnum { | ||
| if reflect.DeepEqual(v, expected) { |
There was a problem hiding this comment.
where does this v come from? I don't see it declared anywhere, are you sure it's valid?
|
Not sure if I'll get to your comments this week or not, but I'll share some thoughts for now.
Yes, once it's in the structure already, you don't know if they set it to a default or not (since required fields are not a pointer). I was thinking of allowing users to pass a set or unset fields list (like the one that
I can take a look. I agree it should work with
I had extended all the tests I wrote before that tested anything outside of code matching to do it for this too. I can look for other tests that do the same and extend them.
I wrote them together to ensure the same tests are always run (when applicable). That way, if you add a feature, you'll see that you should test both. A bug fix would also automatically test both too. Thoughts? |
|
@nolag ok for pts. 2 and 4. pt 1 should probably be fixed and covered by a test that exercises the generated code (I started introducing them, but they are still very few). pt 3 is a bit challenging as I feel we should rethink the flags before proceeding. Should we make them additive (--struct-verify, --unmarshall-validate) or subtractive (--only-model, --no-verify) by default? |
This is useful if the struct is created by the user, or parsed with something like mapstructure and the user wants to verify it against the json schema.