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

Allow not JAX-RS parameters within resource methods #40031

Merged

Conversation

poldinik
Copy link
Contributor

@poldinik poldinik commented Apr 11, 2024

This allows to ignore params that are not related to JAX-RS fields, marking them as SKIPPED in order to use different annotated params with respect JAX-RS api in same method. This feature can be used by extension that generate a dedicate build item to init skipping feature

Copy link

quarkus-bot bot commented Apr 11, 2024

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)
  • title should not contain an issue number (use Fix #1234 in the description instead)
  • title should not start with chore/docs/feat/fix/refactor but be a proper sentence

This message is automatically generated by a bot.

@poldinik poldinik changed the title feat: #39979 Init skipMethodParameter with defined predicate by user Initialization of skipMethodParameter with defined predicate by user Apr 12, 2024
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, let's wait @geoand feedback.


@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
public @interface RestEasyParamsFilter {
Copy link
Member

Choose a reason for hiding this comment

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

I will defer to @geoand on this PR but my guess is that we need a better name without RestEasy in it.

@gsmet gsmet requested a review from geoand April 17, 2024 12:35
@geoand
Copy link
Contributor

geoand commented Apr 17, 2024

I need to think about this because I'm not a fan of running the predicate at build time

@poldinik
Copy link
Contributor Author

I need to think about this because I'm not a fan of running the predicate at build time

If it not feasible having predicate at build time (why? Just to be aligned to on design decision), I could provide an alternative implementation. As we said, it is a draft of PR to provide solution for the use case

@geoand
Copy link
Contributor

geoand commented Apr 17, 2024

(why? Just to be aligned to on design decision)

The reason is that user code is almost never run during build time

@poldinik poldinik force-pushed the draft-of-resteasy-params-skip-filter branch from 7cc8eda to 06081cc Compare April 20, 2024 14:01
@poldinik poldinik changed the title Initialization of skipMethodParameter with defined predicate by user Skip jax rs method params indexing Apr 20, 2024
@poldinik poldinik changed the title Skip jax rs method params indexing Skip indexing of all not JAX-RS method params Apr 20, 2024
@poldinik
Copy link
Contributor Author

poldinik commented Apr 20, 2024

(why? Just to be aligned to on design decision)

The reason is that user code is almost never run during build time

Wondering about your doubts, I have push forced a new solution to achieve same behavior. Now if it is enabled a skipAllNotMethodParameter configuration, the endpoint indexer set as SKIPPED every params that is NOT a JAX-RS params.
The rationale is that there is no reason I guess to delegate the user a specific business logic implementation to filter out params, because you want either param handled as JAX-RS, either not. There is no other option from use case prospective

@poldinik poldinik force-pushed the draft-of-resteasy-params-skip-filter branch 3 times, most recently from 0a23854 to 60fcd08 Compare April 20, 2024 15:05
@geoand
Copy link
Contributor

geoand commented Apr 24, 2024

Seems more reasonable now, thanks.

Is there any chance you can add some sort of test for this?

@poldinik
Copy link
Contributor Author

Seems more reasonable now, thanks.

Is there any chance you can add some sort of test for this?

Yes, I'm going to add tests

@poldinik
Copy link
Contributor Author

Seems more reasonable now, thanks.

Is there any chance you can add some sort of test for this?

I have just added tests.

Moreover, I have done a small refinement about configuration definition: wondering about design of this feature, I guess "skip" is not totally correct in prospective of developer who use the configuration, in fact he could be unaware about how indexing of params is done (and implementation strategy of deployment in general). So the term "skip" is used within the implementation and the configuration is now: quarkus.rest.allow-not-rest-methods-params

@geoand
Copy link
Contributor

geoand commented Apr 25, 2024

Thanks for this.

Now that I think of it more, I don't believe that a config property is the right way to go for this, because no application would ever set it.
To me it makes a lot more sense to have this be a build item that other extensions would generate when they need to have this feature installed.

@poldinik
Copy link
Contributor Author

Thanks for this.

Now that I think of it more, I don't believe that a config property is the right way to go for this, because no application would ever set it.

To me it makes a lot more sense to have this be a build item that other extensions would generate when they need to have this feature installed.

I didn't think to this, but it is more feasible actually. I'll try to write down something in this direction

@poldinik
Copy link
Contributor Author

poldinik commented Apr 27, 2024

Thanks for this.

Now that I think of it more, I don't believe that a config property is the right way to go for this, because no application would ever set it. To me it makes a lot more sense to have this be a build item that other extensions would generate when they need to have this feature installed.

Just committed build item approach. If there is a better package to store new item let me know. Moreover, I have some doubts about if I should edit an existing item or defining a fine-grained item as I have done

@geoand
Copy link
Contributor

geoand commented Apr 29, 2024

Seems reasonable!

Can you please squash the commits?

@poldinik poldinik force-pushed the draft-of-resteasy-params-skip-filter branch from 133fde4 to 3b82d19 Compare April 29, 2024 10:50
@poldinik
Copy link
Contributor Author

poldinik commented Apr 29, 2024

Squashed commits, I have run formatter plugin too


import io.quarkus.builder.item.SimpleBuildItem;

public final class AllowNotRestParametersBuildItem extends SimpleBuildItem {
Copy link
Contributor

Choose a reason for hiding this comment

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

A little Javadoc on what this done would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@poldinik poldinik force-pushed the draft-of-resteasy-params-skip-filter branch from 3b82d19 to c14cf3f Compare April 29, 2024 11:01
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!

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 29, 2024
@gastaldi gastaldi added kind/bugfix and removed area/kubernetes area/hibernate-search Hibernate Search area/dependencies Pull requests that update a dependency file area/qute The template engine area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/cache area/infra-automation anything related to CI, bots, etc. that are used to automated our infrastructure area/liquibase area/grpc gRPC area/redis area/elasticsearch area/graphql area/tracing area/fault-tolerance area/cli Related to quarkus cli (not maven/gradle/etc.) area/adr area/context-propagation area/docstyle issues related for manual docstyle review area/netty labels Apr 29, 2024
Copy link

quarkus-bot bot commented Apr 29, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit db0fce3.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link

github-actions bot commented Apr 29, 2024

🎊 PR Preview ca27e0c has been successfully built and deployed to https://quarkus-pr-main-40031-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

This comment has been minimized.

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 30, 2024
Copy link

quarkus-bot bot commented Apr 30, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit f4ac9ed.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 21

📦 extensions/hibernate-orm/deployment

io.quarkus.hibernate.orm.HibernateHotReloadTestCase.testImportSqlWithContinuousTesting - History

  • Failed to wait for test run 3 State{lastRun=1, running=true, inProgress=false, run=1, passed=1, failed=0, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true} - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Failed to wait for test run 3 State{lastRun=1, running=true, inProgress=false, run=1, passed=1, failed=0, skipped=0, isBrokenOnly=false, isTestOutput=false, isInstrumentationBasedReload=false, isLiveReload=true}
	at io.quarkus.test.ContinuousTestingTestUtils.waitForNextCompletion(ContinuousTestingTestUtils.java:44)
	at io.quarkus.hibernate.orm.HibernateHotReloadTestCase.testImportSqlWithContinuousTesting(HibernateHotReloadTestCase.java:87)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1596)
Caused by: org.awaitility.core.ConditionTimeoutException: Condition returned by method "waitForNextCompletion" in class io.quarkus.test.ContinuousTestingTestUtils was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)

📦 extensions/smallrye-reactive-messaging-kafka/deployment

io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

  • Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

@geoand geoand merged commit c5ac239 into quarkusio:main Apr 30, 2024
45 checks passed
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Apr 30, 2024
@quarkus-bot quarkus-bot bot added this to the 3.11 - main milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants