Skip to content
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

fix: panic on oneof validation #86

Merged
merged 4 commits into from
Dec 6, 2022
Merged

fix: panic on oneof validation #86

merged 4 commits into from
Dec 6, 2022

Conversation

megaflo
Copy link
Contributor

@megaflo megaflo commented Dec 6, 2022

The generated validation for oneof schemas produces an error, if used as a property in another object and left empty (e.g. by omitting that property from the json payload). The cause is a nil initialized interface in the implementation, that gets dereferenced.

This PR adds extra checks to make sure the nil interface is never dereferenced.

Ticket

How Has This Been Verified?

A new test has been added to verify the expected behavior (a validation error is returned for missing oneof fields).

Checklist:

  • The change works as expected.
  • New code can be debugged via logs.
  • I have added tests to cover my changes.
  • I have locally run the tests and all new and existing tests passed.
  • Requires updates to the documentation.
  • I have made the required changes to the documents.

@megaflo megaflo requested a review from a team as a code owner December 6, 2022 12:35
@megaflo megaflo requested review from dmahlow and LucasRoesler and removed request for a team December 6, 2022 12:35
@megaflo
Copy link
Contributor Author

megaflo commented Dec 6, 2022

@LucasRoesler I just realized that this way of fixing the issue effectively makes any oneof field required, as validation will fail, if the property is not provided. Is that something we can live with or should I try to avoid validation of the property if it is not listed as required?

@LucasRoesler
Copy link
Member

@LucasRoesler I just realized that this way of fixing the issue effectively makes any oneof field required, as validation will fail, if the property is not provided. Is that something we can live with or should I try to avoid validation of the property if it is not listed as required?

is it possible to toggle the error behavior based on the not null property?

@LucasRoesler
Copy link
Member

@megaflo actually now that i think about it, i don't think we need to change anything, the required validation should already work as expected, e.g

if you have

    Container:
      type: object
      properties:
        error:
          $ref: "#/components/schemas/Error"

then

func (m Container) Validate() error {
	return validation.Errors{
		"error": validation.Validate(
			m.Error,
		),
	}.Filter()
}

should skip validation is m.Error is empty, this is part of the behavior of the ozzo-validation

LucasRoesler
LucasRoesler previously approved these changes Dec 6, 2022
@LucasRoesler LucasRoesler removed the request for review from dmahlow December 6, 2022 16:12
@megaflo megaflo merged commit 317299d into master Dec 6, 2022
@megaflo megaflo deleted the fix_oneof_validation branch December 6, 2022 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants