-
Notifications
You must be signed in to change notification settings - Fork 43
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
Rule Type schema validation: change library and apply defaults #4953
Conversation
8089c5f
to
56b5a4f
Compare
1e16bb5
to
a4c37a1
Compare
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.
Would it make sense to call applyDefaults
separately instead of making it part of both validation functions?
Since ValidateRuleDef
and ValidateParams
are already separate, I expected ApplyDefaults
to be separate as well.
That might make sense, I just wanted to simplify the API by doing both validation and applying defaults. Else the implementor gets a more verbose setup. |
} | ||
|
||
return fmt.Errorf("invalid json schema: %s", strings.TrimSpace(strings.Join(problems, "\n"))) |
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.
Why TrimSpace
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.
In all honesty, I don't remember. It may not be needed anymore.
return fmt.Errorf("invalid json schema: %s", strings.TrimSpace(strings.Join(problems, "\n"))) | ||
} | ||
|
||
// applyDefaults recursively applies default values from the schema to the object. | ||
func applyDefaults(schema *jsonschema.Schema, obj map[string]any) { |
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.
I wish the library had this function, but it looks like it doesn'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.
No library I found has one 😢
It feels like we should do |
This changes the underlying library to something that's actually maintained: github.com/santhosh-tekuri/jsonschema/v6 It also adds the feature of applying defaults if they were defined in the JSON schema. Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
Signed-off-by: Juan Antonio Osorio <[email protected]>
49a86d6
a4c37a1
to
49a86d6
Compare
Summary
This changes the underlying library to something that's actually
maintained: github.com/santhosh-tekuri/jsonschema/v6
It also adds the feature of applying defaults if they were defined in
the JSON schema.
Closes #1657
Change Type
Mark the type of change your PR introduces:
Testing
Added a relevant unit test.
Review Checklist: