go-jsonschema icon indicating copy to clipboard operation
go-jsonschema copied to clipboard

Allow structs to be self-verifying.

Open nolag opened this issue 1 year ago • 8 comments

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.

nolag avatar Oct 10 '24 16:10 nolag

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.

Files with missing lines Patch % Lines
pkg/generator/verify_formatter.go 73.88% 35 Missing :warning:
...s/data/validation/requiredFields/requiredFields.go 0.00% 15 Missing :warning:
...data/validation/requiredFields/requiredNullable.go 0.00% 8 Missing :warning:
...ta/validation/exclusiveMaximum/exclusiveMaximum.go 57.14% 4 Missing and 2 partials :warning:
...ta/validation/exclusiveMinimum/exclusiveMinimum.go 57.14% 4 Missing and 2 partials :warning:
tests/data/validation/maximum/maximum.go 57.14% 4 Missing and 2 partials :warning:
tests/data/validation/minimum/minimum.go 57.14% 4 Missing and 2 partials :warning:
tests/data/validation/multipleOf/multipleOf.go 57.14% 4 Missing and 2 partials :warning:
...s/data/validation/primitive_defs/primitive_defs.go 70.58% 4 Missing and 1 partial :warning:
tests/data/validation/pattern/pattern.go 55.55% 3 Missing and 1 partial :warning:
... and 2 more
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.

codecov[bot] avatar Oct 10 '24 16:10 codecov[bot]

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!

omissis avatar Oct 30 '24 18:10 omissis

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.

nolag avatar Oct 31 '24 13:10 nolag

Not sure if I'll get to your comments this week or not, but I'll share some thoughts for now.

  1. the generated code for enums in the tests seem to lack a v variable Ack, will fix. This does mean that the tests never checked that compiled file.
  1. was it intended not to check for required in 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.

  1. 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-verify is ignored if --only-models is passed

I can take a look. I agree it should work with --only-models.

  1. 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 in tests/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 avatar Nov 04 '24 17:11 nolag

@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?

omissis avatar Nov 16 '24 10:11 omissis

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.

Files with missing lines Patch % Lines
pkg/generator/verify_formatter.go 67.72% 47 Missing and 4 partials :warning:
...s/data/validation/requiredFields/requiredFields.go 0.00% 15 Missing :warning:
...data/validation/requiredFields/requiredNullable.go 0.00% 10 Missing :warning:
tests/data/validation/multipleOf/multipleOf.go 55.55% 6 Missing and 2 partials :warning:
tests/data/validation/pattern/pattern.go 38.46% 6 Missing and 2 partials :warning:
...ta/validation/exclusiveMaximum/exclusiveMaximum.go 57.14% 4 Missing and 2 partials :warning:
...ta/validation/exclusiveMinimum/exclusiveMinimum.go 57.14% 4 Missing and 2 partials :warning:
tests/data/validation/maximum/maximum.go 57.14% 4 Missing and 2 partials :warning:
tests/data/validation/minimum/minimum.go 57.14% 4 Missing and 2 partials :warning:
...s/data/validation/primitive_defs/primitive_defs.go 70.58% 4 Missing and 1 partial :warning:
... and 4 more
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.

codecov[bot] avatar May 11 '25 20:05 codecov[bot]

@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.

omissis avatar May 11 '25 20:05 omissis

@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.

nolag avatar May 13 '25 13:05 nolag