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

Resteasy Reactive: Support use of optional with list/set/sortedset #24055

Merged
merged 1 commit into from
Mar 2, 2022

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented Mar 2, 2022

Support use when using query params with some collections. Example:

@Path("/list")
    @GET
    public String sayHelloToList(@QueryParam("name") final Optional<List<String>> names) {
        return doSayHelloToCollection(names);
    }

fix #23898

@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 2, 2022

/cc @geoand @Plawn

@Sgitario Sgitario marked this pull request as draft March 2, 2022 11:02
@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 2, 2022

Moving to Draft as this is not fully working yet.

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Thanks!

@Sgitario Sgitario force-pushed the support_options_query branch from 02dc081 to 6e22f6e Compare March 2, 2022 11:14
@Sgitario Sgitario marked this pull request as ready for review March 2, 2022 11:15
@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 2, 2022

Moving to Draft as this is not fully working yet.

Now, the PR is ready to be reviewed.

There is a controversial difference of behaviors here between Resteasy Reactive and Resteasy Classic. Let's imagine we have the following endpoint:

@Path("/list")
@GET
public String sayHelloToList(@QueryParam("name") final Optional<List<String>> names) {
   // ...
}

If no query params are being set, I would expect to receive an optional empty. This is working like this in Resteasy Reactive, but in Resteasy Classic, you'll receive the optional with an empty list of strings.

@geoand
Copy link
Contributor

geoand commented Mar 2, 2022

Optional<List<T>> is always weird if you ask me...

I think the way we have it now is fine, I wouldn't worry too much about maintaining compatibility on this particular point

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 2, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 6e22f6e

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build Build Failures Logs Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-classic/resteasy/deployment 
! Skipped: devtools/bom-descriptor-json docs extensions/agroal/deployment and 309 more

📦 extensions/resteasy-classic/resteasy/deployment

Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.17.1:validate (default) on project quarkus-resteasy-deployment: File '/home/runner/work/quarkus/quarkus/extensions/resteasy-classic/resteasy/deployment/src/test/java/io/quarkus/resteasy/test/QueryParamTest.java' has not been previously formatted. Please format file and commit before running validation!

Support use when using query params with some collections. Example:

```java
@path("/list")
    @get
    public String sayHelloToList(@QueryParam("name") final Optional<List<String>> names) {
        return doSayHelloToCollection(names);
    }
```

fix quarkusio#23898
@Sgitario Sgitario force-pushed the support_options_query branch from 6e22f6e to 9dd7f7b Compare March 2, 2022 11:29
@geoand geoand added triage/backport? triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 2, 2022
Object converted = delegate.convert(parameter);
if (converted != null
&& converted instanceof Collection
&& ((Collection) converted).isEmpty()) {
Copy link

Choose a reason for hiding this comment

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

Is it really necessary to check if the collection is Empty ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To return optional empty when the collection is empty, yes. See my comment here: #24055 (comment)

Copy link

Choose a reason for hiding this comment

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

Oh I see, you are right, It's better this way

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 2, 2022

Failing Jobs - Building 9dd7f7b

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/reactive-messaging-kafka 

📦 integration-tests/reactive-messaging-kafka

io.quarkus.it.kafka.KafkaConnectorTest.testPets - More details - Source on GitHub

java.lang.RuntimeException: 
java.lang.RuntimeException: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
	[error]: Build step io.quarkus.kafka.client.deployment.DevServicesKafkaProcessor#startKafkaDevService threw an exception: java.lang.RuntimeException: org.testcontainers.containers.ContainerLaunchException: Container startup failed

@geoand geoand merged commit 7ca73f3 into quarkusio:main Mar 2, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Mar 2, 2022
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Mar 2, 2022
@Sgitario Sgitario deleted the support_options_query branch March 2, 2022 15:09
@michalszynkiewicz
Copy link
Member

Should we support Optional<List>, etc in the client too? Hmmm... maybe we already do

@Sgitario
Copy link
Contributor Author

Sgitario commented Mar 3, 2022

Should we support Optional<List>, etc in the client too? Hmmm... maybe we already do

As far I could see, the rest client reactive extension already supports it.

@gsmet gsmet modified the milestones: 2.8 - main, 2.7.4.Final Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't use Optional with List with @QueryParam with reasteasy controller
5 participants