Allow structs to be self-verifying.
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.
Codecov Report
Attention: Patch coverage is 66.00660% with 103 lines in your changes missing coverage. Please review.
Project coverage is 72.54%. Comparing base (
d963216) to head (d301e4c). Report is 77 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #293 +/- ##
==========================================
- Coverage 76.58% 72.54% -4.05%
==========================================
Files 24 44 +20
Lines 1892 3136 +1244
==========================================
+ Hits 1449 2275 +826
- Misses 354 675 +321
- Partials 89 186 +97
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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!
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.
Not sure if I'll get to your comments this week or not, but I'll share some thoughts for now.
- the generated code for enums in the tests seem to lack a
vvariable Ack, will fix. This does mean that the tests never checked that compiled file.
- was it intended not to check for
requiredin the verify functions?
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 mapstructure can set for unset variables), or a func that returns if a field was set, but the overhead seemed pretty high. Also, the caller may not know what was set at the time of calling.
- 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 can take a look. I agree it should work with --only-models.
- 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
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 would prefer having two suites of tests, one with the classic validation and one with the struct verify. wdyt?
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?
Codecov Report
:x: Patch coverage is 61.58358% with 131 lines in your changes missing coverage. Please review.
:warning: Please upload report for BASE (main@b662ae3). Learn more about missing BASE report.
Additional details and impacted files
@@ Coverage Diff @@
## main #293 +/- ##
=======================================
Coverage ? 43.49%
=======================================
Files ? 61
Lines ? 6691
Branches ? 0
=======================================
Hits ? 2910
Misses ? 3490
Partials ? 291
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
@nolag I have rebased, fixed the conflicts, and fixed a couple things here and there. Are you still interested in working on this PR? If so, can you take a look at my changes, and provide the additional tests? also interoperability with --only-models still needs to be designed and implemented.
@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?
Sorry, we're moving away from JSON Schema validation in favour of proto so I won't be working on this PR anymore. IMO, unmarshal-validate should imply struct verify, and struct-verify with no-verify doesn't make sense.