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

Use mapping to directly validate mapped schema #338

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

riccardomanfrin
Copy link
Contributor

Commit 5547859 permits, when the discriminator and mapping are used to visitJSON for the specific Schema without losing time, and to return the specific error instead of the generic one returned when mapping is not specified.

@riccardomanfrin riccardomanfrin changed the title As per commit message Use mapping to directly validate mapped schema Apr 8, 2021
@fenollp
Copy link
Collaborator

fenollp commented Apr 12, 2021

Cool so can't the duplicate code be removed? and if not could you avoid the duplication by introducing a function please.

openapi3/schema.go Outdated Show resolved Hide resolved
(cherry picked from commit d743e52)
(cherry picked from commit 818b0df)
(cherry picked from commit 7b15e5e)
This reverts commit 1286d06.

# Conflicts:
#	openapi3filter/validation_discriminator_test.go

(cherry picked from commit a6bc0e1)
(cherry picked from commit 011aa82)
(cherry picked from commit 56c30a7)
(cherry picked from commit 393fdec)
(cherry picked from commit a3ea3ba)
(cherry picked from commit dfd1108)
(cherry picked from commit 1b40d29)
(cherry picked from commit f4f135f)
openapi3filter/validate_request_test.go Outdated Show resolved Hide resolved
openapi3/schema.go Show resolved Hide resolved
openapi3/schema.go Outdated Show resolved Hide resolved
openapi3/schema.go Outdated Show resolved Hide resolved
openapi3/schema.go Outdated Show resolved Hide resolved
@fenollp
Copy link
Collaborator

fenollp commented Apr 23, 2021

Just a few nits but this is otherwise correct.

@fenollp
Copy link
Collaborator

fenollp commented Apr 23, 2021

Looks like the test fails to print the expected error. You can use this to reproduce:

go test -v -run Example ./openapi3filter

This is what schema.go:856 else is for

Spec quote:

    ``It is implied, that the property to which discriminator refers, contains the
    name of the target schema. In the example above, the objectType property should
    contain either simpleObject, or complexObject string.''*/
@riccardomanfrin
Copy link
Contributor Author

Looks like the test fails to print the expected error. You can use this to reproduce:

go test -v -run Example ./openapi3filter

I didn't even know about this notation .. I searched for where Example was called, expecting the validation would come from outside or something ..didn't know it was in the comments..

So that is where the else would come into play (from your original question)

@riccardomanfrin riccardomanfrin requested a review from fenollp April 26, 2021 07:57
// "bark": true,
// "breed": "Dingo",
// "pet_type": "Cat"
// }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the request data should be invalid per the provided schema. What am I missing?

The test used to not pass because there was no output, because no error is returned. Are you sure this should not output anything?

Copy link
Contributor Author

@riccardomanfrin riccardomanfrin Apr 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This relates to your original question about not being sure regarding the presence of the "else" statement:

Quoting from https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

"It is implied, that the property to which discriminator refers, contains the name of the target schema. In the example above, the objectType property should contain either simpleObject, or complexObject string. If the property values do not match the schema names, you can map the values to the names. To do this, use the discriminator/mapping keyword:"

I'm interpreting this as saying that if you don't specify the mapping the default mapping is based on the names of the objects included in the oneOf list: simpleObject, complexObject in the spec doc, Dog or Cat in the Example() from validate_request_test.go.

Because of that "else", ValidateRequest() is not anymore returning an error in the Example() code, and therefore I had to modify the output to be empty, accordingly.

Let me know if you believe I'm getting it all wrong or it makes sense.

Side note: the fact that now a "barking cat" is legitimate in the Example() can be ruled out with other means (additionalProperties and required keyword props). This would possibly make the Example more meaningful (and getting it back to fail again).

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.

2 participants