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

Rest-Client-Reactive: Allow FormParams to be used in BeanParams #23099

Merged
merged 1 commit into from
Jan 29, 2022

Conversation

fwippe
Copy link
Contributor

@fwippe fwippe commented Jan 21, 2022

This PR ensures that @FormParams are taken into account when parsing @BeanParams at Rest Client Reactive interface stubs.

@geoand I certainly imagined the change to be simpler. Please take a closer look at the changes regarding the BeanParamParser; I tried to simplify the parameter extraction, hopefully easing the way for upcoming additional param type integrations.

@geoand
Copy link
Contributor

geoand commented Jan 21, 2022

Thanks!

As this is for the client, I'll defer to @michalszynkiewicz for the review

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 21, 2022

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

Failing Jobs - Building 4cf0ec9

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

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 11 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

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

@fwippe fwippe force-pushed the beanparam_formparam branch from 4cf0ec9 to 58cca89 Compare January 21, 2022 16:54
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 21, 2022

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

Failing Jobs - Building 58cca89

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

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 11 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

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

@fwippe fwippe force-pushed the beanparam_formparam branch 3 times, most recently from ce9aafd to db94e76 Compare January 23, 2022 20:25
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 23, 2022

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

Failing Jobs - Building db94e76

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.MultiModuleIncludedBuildTest.main line 24 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 24, 2022

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

Failing Jobs - Building c19610f

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Extract Maven Repo ⚠️ Check → Logs Raw logs
Gradle Tests - JDK 11 Windows Extract Maven Repo ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Extract Maven Repo ⚠️ Check → Logs Raw logs
JVM Tests - JDK 11 Windows Extract Maven Repo ⚠️ Check → Logs Raw logs
JVM Tests - JDK 17 Extract Maven Repo ⚠️ Check → Logs Raw logs
MicroProfile TCKs Tests Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data1 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data2 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data3 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data4 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data5 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data6 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Data7 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - HTTP Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Messaging1 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Misc1 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Misc3 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Misc4 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Security2 Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - Spring Extract Maven Repo ⚠️ Check → Logs Raw logs
Native Tests - gRPC Extract Maven Repo ⚠️ Check → Logs Raw logs

@fwippe fwippe force-pushed the beanparam_formparam branch from 49ba198 to 759e162 Compare January 24, 2022 08:09
@fwippe
Copy link
Contributor Author

fwippe commented Jan 24, 2022

Gradle build failed (probably not because of my changes). I rebased to the latest commit, let's see if it helps...
Update: all checks successful 👍

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 24, 2022

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 49ba198

Status Name Step Failures Logs Raw logs
Initial JDK 11 Build ⚠️ Check → Logs Raw logs
Attach pull request number ⚠️ Check → Logs Raw logs
CI Sanity Check ⚠️ Check → Logs Raw logs

@fwippe fwippe force-pushed the beanparam_formparam branch 2 times, most recently from 7dd7f7d to 87b1879 Compare January 26, 2022 08:54
@fwippe
Copy link
Contributor Author

fwippe commented Jan 26, 2022

Update: Added cycle detection for recursive @BeanParam parsing and took recent changes of @geoand for issue #23034 into account

@fwippe
Copy link
Contributor Author

fwippe commented Jan 27, 2022

@michalszynkiewicz Have you perhaps had a chance to look at it yet? What do you think? Anything missing? Certainly, I'm open for discussion!
Considering that the @BeanParam documentation uses @FormParams as example, this PR covers a fairly basic need.

@michalszynkiewicz
Copy link
Member

It looks good to me, it would be great to have a test for form param + param converter + bean param. But I think it can be added in a separate PR. Let me know if you have time to add it to this one soon, otherwise I'll merge it

@fwippe fwippe force-pushed the beanparam_formparam branch 2 times, most recently from 8a6c08e to a8bc3a3 Compare January 28, 2022 14:58
@fwippe
Copy link
Contributor Author

fwippe commented Jan 28, 2022

Included convertable @FormParam type in BeanFormParamTest

@quarkus-bot
Copy link

quarkus-bot bot commented Jan 28, 2022

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

Failing Jobs - Building a8bc3a3

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

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 11 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

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

@fwippe fwippe force-pushed the beanparam_formparam branch from a8bc3a3 to b849cfc Compare January 28, 2022 15:20
@quarkus-bot
Copy link

quarkus-bot bot commented Jan 28, 2022

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

Failing Jobs - Building b849cfc

Status Name Step Failures Logs Raw logs
Gradle Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ Gradle Tests - JDK 11 Windows #

- Failing: integration-tests/gradle 

📦 integration-tests/gradle

io.quarkus.gradle.devmode.ModuleWithParentDependencyDevModeTest.main line 14 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.test.devmode.util.DevModeTestUtils that uses java.util.function.Supplier, java.util.function.Supplierjava.util.concurrent.atomic.AtomicReference, java.util.concurrent.atomic.AtomicReferencejava.lang.String, java.lang.Stringboolean was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@fwippe
Copy link
Contributor Author

fwippe commented Jan 28, 2022

The Gradle build failed, but I don't think it's related to this PR.

@fwippe fwippe force-pushed the beanparam_formparam branch from b849cfc to b278da0 Compare January 29, 2022 11:27
@fwippe fwippe force-pushed the beanparam_formparam branch from b278da0 to bb8091d Compare January 29, 2022 11:29
@fwippe
Copy link
Contributor Author

fwippe commented Jan 29, 2022

I rebased to latest HEAD, all checks passed

@michalszynkiewicz michalszynkiewicz merged commit 92c0468 into quarkusio:main Jan 29, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Jan 29, 2022
@michalszynkiewicz
Copy link
Member

thanks @fwippe !

@geoand
Copy link
Contributor

geoand commented Jan 29, 2022

@michalszynkiewicz I assume we want to backport this to 2.7, right?

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.

4 participants