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

ArC: improve validation of interceptor method signatures #38796

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Feb 15, 2024

No description provided.

@Ladicek Ladicek requested review from mkouba and manovotn February 15, 2024 13:11
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 15, 2024

I think we might wanna backport this to 3.7/3.8, WDYT @mkouba?

@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Feb 15, 2024
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

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

Looks good but shouldn't we also add a test for @AroundInvoke having void return type?

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 15, 2024

Ah I intended to do that, but forgot! Thanks, will add.

@Ladicek Ladicek force-pushed the arc-improve-interceptor-validation branch from 2b9ae4d to 7feb424 Compare February 15, 2024 14:49
@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 15, 2024

Done. Thanks for reminding me!

@manovotn manovotn added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 15, 2024
Copy link

quarkus-bot bot commented Feb 15, 2024

Status for workflow Quarkus CI

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

✅ 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.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

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

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

  • Assertion condition defined as a 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 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)

@Ladicek Ladicek merged commit f572b39 into quarkusio:main Feb 16, 2024
49 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 16, 2024
@Ladicek Ladicek deleted the arc-improve-interceptor-validation branch February 16, 2024 08:25
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Feb 16, 2024
@gsmet
Copy link
Member

gsmet commented Feb 19, 2024

I'm not sure about backporting this one? My understanding is that apps that were somehow building without errors will fail, right? Granted, they wouldn't work well but still.
I think we are a bit late in this cycle to do that.

Unless I misunderstood something?
(I could be convinced if you have a case)

@Ladicek
Copy link
Contributor Author

Ladicek commented Feb 19, 2024

Not a huge deal if we don't backport this, we already had a mostly good (say 95%) validation in place, this just tightens it up (to 100%).

@mkouba
Copy link
Contributor

mkouba commented Feb 20, 2024

@gsmet I agree with Ladislav. So let's not backport this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/arc Issue related to ARC (dependency injection) triage/flaky-test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants