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

Register @ConfigMappings directly into the Config builder #18497

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

radcortez
Copy link
Member

@radcortez radcortez commented Jul 7, 2021

Initial work to register the mappings directly with the builder.

@quarkus-bot quarkus-bot bot added area/arc Issue related to ARC (dependency injection) area/config area/core area/dependencies Pull requests that update a dependency file area/hibernate-validator Hibernate Validator labels Jul 7, 2021
@radcortez radcortez force-pushed the fix-18333 branch 2 times, most recently from f32a1f0 to 6ddda8f Compare July 15, 2021 17:49
@radcortez radcortez marked this pull request as ready for review July 15, 2021 17:49
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 15, 2021

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 6ddda8f

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status Name Step Test failures Logs Raw logs
CI Sanity Check Build ⚠️ Check → Logs Raw logs

@quarkus-bot
Copy link

quarkus-bot bot commented Jul 21, 2021

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

Failing Jobs - Building 5590255

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 16

@gsmet
Copy link
Member

gsmet commented Aug 2, 2021

I rebased it as it was a bit old.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 2, 2021
@@ -1,4 +1,4 @@
package io.quarkus.arc.deployment;
package io.quarkus.deployment.builditem;
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should just deprecate the old one for 2.2? Or we are sure it's not used anywhere in our extension ecosystem?

(deprecating a build item is easy, you just consume the old ones and produce the new ones)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it is ok to rename, this is only intended for internal usage.

This raises an interesting question. Do we have a way to distinguish which build step may be used safely on the ecosystem and which are for internal only? Unfortunately, a regular access modifier is not enough.

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't.

@quarkus-bot
Copy link

quarkus-bot bot commented Aug 2, 2021

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

Failing Jobs - Building 6b405a0

Status Name Step Test failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Test failures Logs Raw logs
✔️ JVM Tests - JDK 16

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

io.quarkus.resteasy.reactive.server.test.simple.SimpleQuarkusRestTestCase.testFilters line 141 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting ArrayList:
  []
to contain only:
  ["authentication-authorization-default"]
but could not find the following element(s):
  ["authentication-authorization-default"]

	at io.quarkus.resteasy.reactive.server.test.simple.SimpleQuarkusRestTestCase.testFilters(SimpleQuarkusRestTestCase.java:141)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:363)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:331)
	at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
	at org.junit.ju...

@gsmet
Copy link
Member

gsmet commented Aug 11, 2021

@radcortez can you please ping me here when this is ready, I keep forgetting about it. Thanks!

@radcortez
Copy link
Member Author

@radcortez can you please ping me here when this is ready, I keep forgetting about it. Thanks!

Yes, it is ready, I've just rebased with main.

@gsmet gsmet merged commit c46f1f8 into quarkusio:main Aug 12, 2021
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 12, 2021
@quarkus-bot quarkus-bot bot added this to the 2.2 - main milestone Aug 12, 2021
@marcelstoer
Copy link
Contributor

marcelstoer commented Aug 27, 2021

@radcortez

Should this now also allow for constructor injection of configs into a an exception mapper? I was getting the same error as reported in #18333 with my exception mapper below.

@Provider
@ApplicationScoped
class RouteExceptionInterceptor(config: MyConfig) : ExceptionMapper<Exception> {

Since I found this I bumped Quarkus to 2.2.0.CR1. The app still does not start but the config error is now different. Hence, I was wondering if this indicates this issue is not fully fixed yet OR whether I am having a different problem.

Should this now also allow for constructor injection of configs into a an exception mapper?

Yes, this PR fixes this as well. Thanks!

@radcortez
Copy link
Member Author

Yes

@radcortez

Should this now also allow for constructor injection of configs into a an exception mapper? I was getting the same error as reported in #18333 with my exception mapper below.

@Provider
@ApplicationScoped
class RouteExceptionInterceptor(config: MyConfig) : ExceptionMapper<Exception> {

Since I found this I bumped Quarkus to 2.2.0.CR1. The app still does not start but the config error is now different. Hence, I was wondering if this indicates this issue is not fully fixed yet OR whether I am having a different problem.

Should this now also allow for constructor injection of configs into a an exception mapper?

Yes, this PR fixes this as well. Thanks!

Yes, but unfortunately we need to revert this because it is creating other issues: #19937

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) area/config area/core area/dependencies Pull requests that update a dependency file area/hibernate-validator Hibernate Validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Injecting ConfigMapping in resteasy containerrequestfilter causes NoSuchElementException
3 participants