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

Correct how we process injection point transformers for SR CP #38841

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

manovotn
Copy link
Contributor

@manovotn manovotn commented Feb 18, 2024

This fixes how we process injection points for SR CP processor where we were incorrectly expecting AnnotationTarget for method param while we should be operating on just method.
There was also a bug in Arc that, for a method injection point, returned a set of qualifiers belonging to the whole method instead of just given parameter.

I have yet to take a closer look at other usages in Quarkus which may have similar issues - one of which is the grpc processor I mentioned in the original issue.

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/context-propagation area/smallrye labels Feb 18, 2024

This comment has been minimized.

@geoand geoand requested a review from mkouba February 19, 2024 07:45
@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Feb 19, 2024
@manovotn
Copy link
Contributor Author

manovotn commented Feb 19, 2024

I have added a comment that also alters gRPC processor which is incorrectly expecting method param as annotation target.
I quickly scanned other IP transformers as well but those look OK.

With that, it should be review ready.

This comment has been minimized.

Copy link
Contributor

@Ladicek Ladicek left a comment

Choose a reason for hiding this comment

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

I can't exactly say I understand the ConProp extension, but the rest LGTM.

@gsmet
Copy link
Member

gsmet commented Feb 20, 2024

@mkouba I let you review that one. Also we need to decide if it's a good idea to backport it or not. I'm on the verge tbh.

@manovotn
Copy link
Contributor Author

I've used the builder pattern for AnnotationInstance as per @Ladicek's suggestion and I've squashed the commits into one.

@manovotn
Copy link
Contributor Author

manovotn commented Feb 20, 2024

@mkouba I let you review that one. Also we need to decide if it's a good idea to backport it or not. I'm on the verge tbh.

Well, it is a rare use case - without this fix it has never worked (if injected as method param) since it's addition and nobody encountered it so far so I don't think it's anyhow crucial.

Actually, I just realized that the bit I fixed in Arc codebase might be worth it as that fixes how you can apply injection point transformers on method params in general 🤔

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Feb 20, 2024

@manovotn looks like DependentBeanJobTest.testDependentBeanJobDestroyed is being flaky

@manovotn
Copy link
Contributor Author

@manovotn looks like DependentBeanJobTest.testDependentBeanJobDestroyed is being flaky

Noticed, already working on it; I will send a separate PR.

Correct gRPC injection point transformer to expect method AnnotationTarget instead of method param.
@manovotn
Copy link
Contributor Author

This should be good to merge but just to be safe and get proper CI result, I've rebased the PR onto main.

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

The Quarkus CI / Native Tests - Data5 (pull_request) doesn't seem related and I am seeing the same failure on other PRs as well.

Copy link

quarkus-bot bot commented Feb 21, 2024

Status for workflow Quarkus CI

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

Failing Jobs

Status Name Step Failures Logs Raw logs Build scan
Native Tests - Data5 Build Failures Logs Raw logs 🚧

Full information is available in the Build summary check run.

Failures

⚙️ Native Tests - Data5 #

- Failing: integration-tests/jpa-postgresql-withxml 

📦 integration-tests/jpa-postgresql-withxml

io.quarkus.it.jpa.postgresql.ImageMetricsITCase.verifyImageMetrics line 15 - History - More details - Source on GitHub

org.opentest4j.AssertionFailedError: Expected analysis_results.fields.reflection to be within range [163 +- 3%] but was 168 ==> expected: <true> but was: <false>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)
	at org.junit.jupiter.api.AssertTrue.failNotTrue(AssertTrue.java:63)
	at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:36)
	at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:214)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.assertValueWithinRange(NativeBuildOutputExtension.java:90)
	at io.quarkus.test.junit.nativeimage.NativeBuildOutputExtension.lambda$verifyImageMetrics$0(NativeBuildOutputExtension.java:66)

Flaky tests - Develocity

⚙️ Maven Tests - JDK 17 Windows

📦 integration-tests/maven

io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch - History

  • Cannot delete directory: target\test-classes\projects\project-generation - java.lang.RuntimeException
java.lang.RuntimeException: Cannot delete directory: target\test-classes\projects\project-generation
	at io.quarkus.maven.it.MojoTestBase.initEmptyProject(MojoTestBase.java:72)
	at io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch(CreateProjectMojoIT.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.apache.commons.io.IOExceptionList: 1 exception(s): [org.apache.commons.io.IOIndexedException: IOException #0: Cannot delete file: target\test-classes\projects\project-generation\build-create-project-generation.log]
	at org.apache.commons.io.IOExceptionList.checkEmpty(IOExceptionList.java:50)

io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch - History

  • Cannot delete directory: target\test-classes\projects\project-generation - java.lang.RuntimeException
java.lang.RuntimeException: Cannot delete directory: target\test-classes\projects\project-generation
	at io.quarkus.maven.it.MojoTestBase.initEmptyProject(MojoTestBase.java:72)
	at io.quarkus.maven.it.CreateProjectMojoIT.testProjectGenerationFromScratch(CreateProjectMojoIT.java:60)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
Caused by: org.apache.commons.io.IOExceptionList: 1 exception(s): [org.apache.commons.io.IOIndexedException: IOException #0: Cannot delete file: target\test-classes\projects\project-generation\build-create-project-generation.log]
	at org.apache.commons.io.IOExceptionList.checkEmpty(IOExceptionList.java:50)

@manovotn manovotn merged commit 79e4355 into quarkusio:main Feb 21, 2024
49 of 50 checks passed
@manovotn manovotn deleted the issue38825 branch February 21, 2024 15:55
@quarkus-bot quarkus-bot bot added this to the 3.9 - main milestone Feb 21, 2024
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Feb 21, 2024
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.

ManagedExecutorConfig does not seem to be working when using constructor injection
4 participants