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 #1191 Validation error on optional exploded query #1192

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

Conversation

lwlee2608
Copy link
Contributor

No description provided.

@slinkydeveloper slinkydeveloper self-requested a review March 8, 2019 08:30
@slinkydeveloper slinkydeveloper added the component/api-contract/bug Bugs for vertx-web-api-contract label Mar 8, 2019
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.

Thank you for your contribution!
I think that this bug is a bit tricky to solve as the actual validation package works. Now the exploded object fields are handled as separated query parameters because I left to ValidationHandler (look in BaseValidationHandler) the task of extracting the parameter from parameters list. This will change in 4.0
To care about all required keywords (both query parameter level and inner object parameter level) we must create a kind of "dependency" that correlates the three validation rules, and it seems to me very very awkward. I prefer to stick with that bug, as in 4.0 this will be easily solved


String requestURI = "/query/form/optional/explode/object?G=200&B=150&alpha=50";

testEmptyRequestWithJSONObjectResponse(HttpMethod.GET, requestURI, 200, "OK", new JsonObject("{\"G\":\"200\",\"B\":\"150\",\"alpha\":50}"));
Copy link
Member

@slinkydeveloper slinkydeveloper Mar 8, 2019

Choose a reason for hiding this comment

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

This test is wrong! The required parameters for ColorObject are R, G, and B, so this should return a 400 Bad Request

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ya.. I basically turn a 'strict' bug into a 'lenient' bug. Because at least it allows me to query without any parameters.
I'll see what I can do to properly fix it.

Copy link
Member

Choose a reason for hiding this comment

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

What if, as workaround, you just use separated query parameters as optional?

Copy link
Contributor Author

@lwlee2608 lwlee2608 Mar 8, 2019

Choose a reason for hiding this comment

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

Unfortunately we are required to use the exact spec file defined by 3GPP.

…e ensure all required params of an optional exploded object must be absence

Signed-off-by: lwlee2608 <[email protected]>
@lwlee2608
Copy link
Contributor Author

@slinkydeveloper I've address the issue. Exploded parameter fields will have some sort of "dependency" on other parameter fields that belong to the same parent Object.

@slinkydeveloper
Copy link
Member

This is exactly what I would like to avoid. A lot of messy replicated code and it doesn't completely address the issue (It should work for header and cookie params too)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/api-contract/bug Bugs for vertx-web-api-contract to review
Development

Successfully merging this pull request may close these issues.

4 participants