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.

Sign up for GitHub

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

Jump to bottom

Schema#getDefaultValue() throws NoSyncValidationException if it is a RefSchema #1816

Merged
merged 2 commits into from
Mar 1, 2021
Merged

Schema#getDefaultValue() throws NoSyncValidationException if it is a RefSchema #1816

merged 2 commits into from
Mar 1, 2021

Conversation

DemonicTutor
Copy link
Contributor

given a yaml:

paths:
  /tryout:
    get:
      operationId: tryout
      parameters:
        - in: query
          name: namespace
          required: false
          schema:
            $ref: '#/components/schemas/namespace'

components:
  schemas:
    namespace:
      type: string
      minLength: 1

using io.vertx.ext.web.openapi.RouterBuilder and sending GET /tryout the schemaValidation will blow up with the noted exception because namespace is null and calling getDefaultValue() on the schema before any validation fails because its a RefSchema and has not been resolved yet.

slinkydeveloper
slinkydeveloper reviewed Dec 14, 2020
View reviewed changes
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

I see the problem, however I have 2 concerns with this solution:

I think we need to find another solution to the problem, I'll try to come up with something soon

...web-validation/src/main/java/io/vertx/ext/web/validation/impl/validator/SchemaValidator.java Outdated
if (s.isSync()) {
return Future.succeededFuture(s.getDefaultValue());
}
return s.validateAsync(null) // validate a dummy to trigger syncing
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't deep solve the references

@DemonicTutor
Copy link
Contributor Author

@slinkydeveloper

well its kind of a quick-fix.

From my point of view getDefaultValue() in Schema should have been async equally to validate but that would have been a more involved change.

Tthis breaking change within the web-validation i would consider acceptable because it should have been a Future in the first place - wouldnt you agree?

@vietj
Copy link
Contributor

vietj commented Dec 14, 2020 via email

@DemonicTutor
Copy link
Contributor Author

@vietj you agree with my assesment regarding Future<> getDefaultValue() ?

@slinkydeveloper as far as i understood the vertx-schema implementation triggering a validation by i.e. s.validateAsync(null) does resolve the RefSchema - what did i miss here?

@vietj
Copy link
Contributor

vietj commented Dec 15, 2020 via email

@slinkydeveloper
Copy link
Member

@slinkydeveloper as far as i understood the vertx-schema implementation triggering a validation by i.e. s.validateAsync(null) does resolve the RefSchema - what did i miss here?

It does, but not deeply. Which means that if you have a ref schema, which references to another schema that contains the property keyword which contains ref schemas, than that won't be solved, because you're passing null so the validator will reject it before going through the properties ref schemas.

In fact, that validateAsync(null) is exactly what's happening in the validator: it gets the parameter, but there is no parameter, so validation is invoked with null argument. Then it checks if there is a default value and it fails with NoSyncValidationException.

One idea I have to fix it is to convert these functions in vertx-json-schema async: https://github.com/eclipse-vertx/vertx-json-schema/blob/master/src/main/java/io/vertx/json/schema/Schema.java#L88

@DemonicTutor
Copy link
Contributor Author

DemonicTutor commented Dec 16, 2020 •
edited
Loading

hmm... if i remember correctly this validateAsync(null) does not happen because there is a if (null!=value) guard.

therefore my change of actually calling it makes the difference that it solves the $ref and for our schemas it works. But those are "relatively" simpel selfcontained single-file schemas with maybe a nesting of up to 3-4 $ref.

If i would go for a "correct fix" i would try to only provide async methods from vertx-schema but that would really break the API - on the other hand i would argue that having sync/async if sync just works in "some specific cases" is a confusing API and leads to more headaches in the long term than breaking it. ¯\_(ツ)_/¯

@slinkydeveloper
Copy link
Member

therefore my change of actually calling it makes the difference that it solves the $ref and for our schemas it works. But those are "relatively" simpel selfcontained single-file schemas with maybe a nesting of up to 3-4 $ref.

That's the issue, a 2 nested schema won't be fixed by this PR.

If i would go for a "correct fix" i would try to only provide async methods from vertx-schema but that would really break the API - on the other hand i would argue that having sync/async if sync just works in "some specific cases" is a confusing API and leads to more headaches in the long term than breaking it.

Yeah I agree, the whole default thing is weird and maybe it should be properly get done in vertx-json-schema first, and then benefit here

@DemonicTutor
Copy link
Contributor Author

@vietj @slinkydeveloper

Hey!

Do you have any update on this issue?

For our own needs ill apply my patch again so we are not blocked currently :)

@slinkydeveloper
Copy link
Member

@DemonicTutor not yet unfortunately, I'll try to work on it asap from the vertx-json-schema side

@slinkydeveloper slinkydeveloper mentioned this pull request Feb 8, 2021
Closed
@slinkydeveloper
Copy link
Member

@DemonicTutor Can you check this PR? eclipse-vertx/vertx-json-schema#34 Also check out the added test, which should check your use case

@DemonicTutor
Copy link
Contributor Author

@slinkydeveloper Thanks! i will have a look ASAP and let you know

@DemonicTutor
fix: Schema#getDefaultValue() throws NoSyncValidationException if it …
09b9092 
…is a RefSchema

Initialize the RefSchema by calling #validateAsync(null) first which causes it to internally resolve the schema

Signed-off-by: Markus Spika <markus.spika@gmail.com>
@DemonicTutor
update to vertx-json-schema PR eclipse-vertx/vertx-json-schema#34
bd69004 
Signed-off-by: Markus Spika <markus.spika@gmail.com>
@DemonicTutor
Copy link
Contributor Author

@slinkydeveloper finally found the time to try it and it worked for me (with this update)

updated my PR to accomodate your changes - thats how it should look right?

slinkydeveloper
slinkydeveloper approved these changes Mar 1, 2021
View reviewed changes
Copy link
Member

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

I've merged the PR in json schema, let's wait for the snapshot to build and i'll retrigger tests here

@slinkydeveloper slinkydeveloper added this to the 4.1.0 milestone Mar 1, 2021
@slinkydeveloper slinkydeveloper merged commit f173a34 into vert-x3:master Mar 1, 2021
@slinkydeveloper
Copy link
Member

@DemonicTutor thank you so much for your help!

@DemonicTutor
Copy link
Contributor Author

@slinkydeveloper awesome ! :)

@DemonicTutor DemonicTutor deleted the issues/NoSyncValidationException branch March 1, 2021 19:40
@slinkydeveloper slinkydeveloper mentioned this pull request Mar 4, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slinkydeveloper slinkydeveloper slinkydeveloper approved these changes

Assignees
No one assigned
Labels
bug component/openapi
Milestone
4.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@DemonicTutor @vietj @slinkydeveloper