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

Require requestBody by default #1858

Merged
merged 5 commits into from
Jun 6, 2024

Conversation

Azquelt
Copy link
Contributor

@Azquelt Azquelt commented May 31, 2024

When generating a requestBody for a resource method or from an annotation, it should be required by default, unless something explicitly indicates that it's not required.

The change affects generation of requestBody only when scanning code. If there's an operation which is annotated @Consumes, the generated requestBody will have required = true, unless the user annotates the method with @RequestBody(required = false).

However, if the user calls OASFactory.createRequestBody in a filter or reader and doesn't call setRequired, then the output will not include required = true.

Implements eclipse/microprofile-open-api#627

Also delete a bunch of test json files which weren't being used.

@Azquelt
Copy link
Contributor Author

Azquelt commented May 31, 2024

This was a bit tricky to implement.

There are two code paths where we read create a request body during code scanning:

  • When @RequestBody is used as a child of @CallbackOperation or @Components
  • When scanning a resource method

The first case is a simple read of the annotation with no other changes made.

The second case reads the annotation if it's present and then combines it with more information that can be discovered from the method. This scanning code assumes that if a property of RequestBody is not null, then it's been set by an earlier part of the scanning process and shouldn't be overwritten.

If we set required = true when reading the annotation, we lose the ability to query whether something else has set the property.

If we try to set required = true at the end of code scanning, we end up having to do that everywhere the annotation is read - in AnnotationScanner, ComponentsIO and CallbackOperationIO which feels like something those IO classes shouldn't really have to deal with.

That's why I ended up extending RequestBodyImpl to allow a default value to be set and to query whether any earlier code has called setRequired().

I'm still not super happy about this approach, it wouldn't scale very well if there were lots of properties with a default value and it means required has to be treated separately. I do wonder whether the current approach of "has this been set already" is really the right one. Some of the discussion in eclipse/microprofile-open-api#356 suggests that having a property on an annotation set to the default value should be treated the same as not setting it at all which is at odds with the current scanning code.

@MikeEdgar MikeEdgar added this to the 4.0.0 milestone Jun 5, 2024
@Azquelt
Copy link
Contributor Author

Azquelt commented Jun 5, 2024

I'm a bit surprised that the 4.0 TCK is passing without eclipse/microprofile-open-api#627 having been merged. I guess that just shows that there was a gap in the TCK coverage.

Azquelt added 5 commits June 6, 2024 11:38
Now that requestBody is required by default, update all the tests to
expect that.
When generating a requestBody for a resource method or from an
annotation, it should be required by default, unless something
explicitly indicates that it's not required.
@Azquelt Azquelt force-pushed the requestbody-default-required branch from 9f64f10 to a5376ab Compare June 6, 2024 10:41
@Azquelt
Copy link
Contributor Author

Azquelt commented Jun 6, 2024

Rebased on main-4.0.

@MikeEdgar MikeEdgar merged commit 1eb6dc7 into smallrye:main-4.0 Jun 6, 2024
3 of 6 checks passed
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