Move BasicAuthenticationMechanism to synthetic bean #45442

geoand · 2025-01-08T14:00:36Z

No description provided.

quarkus-bot · 2025-01-08T14:00:41Z

Thanks for your pull request!

Your pull request does not follow our editorial rules. Could you have a look?

description should not be empty, describe your intent or provide links to the issues this PR is fixing (using Fixes #NNNNN) or changelogs

_{This message is automatically generated by a bot.}

michalvavrik

I actually moved it from synthetic bean to normal one few releases back. I thought it was better to have normal beans as they are ready sooner in case someone injected HTTP authenticator very early as it is a singleton I think.

But it's hardly important, I agree it is easier to use defautlBean() rather than annotation transformer. I don't mind changes, but I'd like to check - the reason why you are proposing this is to avoid transformer because it is requires more operations (like the annotation transformer predicate is run many times), correct?

michalvavrik · 2025-01-08T14:06:29Z

...tx-http/deployment/src/main/java/io/quarkus/vertx/http/deployment/HttpSecurityProcessor.java

        if (buildTimeConfig.auth.basic.isPresent() && buildTimeConfig.auth.basic.get()) {
            securityInformationProducer.produce(SecurityInformationBuildItem.BASIC());
        }

-        return AdditionalBeanBuildItem.builder().setUnremovable().addBeanClass(BasicAuthenticationMechanism.class).build();
+        SyntheticBeanBuildItem.ExtendedBeanConfigurator configurator = SyntheticBeanBuildItem


I think default bean scope is @Dependent and you dropped @Singleton so you are making it dependent. It could be actually good idea, but first, I'd need to inspect our code and probably make same change for all the auth mechanisms. Is this intentional? if not, let's keep it a singleton for now.

Yeah, I picked up on that and have the change done locally

BTW I glanced at current impl. and I actually think all the HTTP auth mechanisms would be in 99 % cases better off as dependent (sometimes users can inject them if they provide alternatives, but how many users does it?), I think we just use them once. I'll re-check later this week if I didn't miss something and open PR when Sergey is back.

Provided that it is worse to have beans we don't need at CDI container, because these java class instances will live anyway. And with the dependent, we won't be proxying beans either. I am not sure if it is worth it.

geoand · 2025-01-08T14:11:54Z

I agree it is easier to use defautlBean() rather than annotation transformer.

Yeah, the synthetic bean is much better for this kind of thing.

Oddly enough the tests are now failing, so I need to check and see what's going on...

michalvavrik · 2025-01-08T14:12:38Z

Ok @geoand , we can do it then.

geoand · 2025-01-08T14:20:40Z

👍🏽

PR updated

michalvavrik

LGTM

quarkus-bot · 2025-01-09T19:50:51Z

Status for workflow `Quarkus CI`

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

✅ 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 17 Windows

📦 integration-tests/opentelemetry-quickstart

✖ io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled - History

Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled(OpenTelemetryDisabledTest.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest was not fulfilled within 200 milliseconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryDisabledTest.buildTimeDisabled(OpenTelemetryDisabledTest.java:29)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

geoand requested a review from michalvavrik January 8, 2025 14:00

quarkus-bot bot added the area/vertx label Jan 8, 2025

geoand marked this pull request as draft January 8, 2025 14:01

michalvavrik reviewed Jan 8, 2025

View reviewed changes

geoand force-pushed the http-mech-bean branch from 3fb9e4c to 7bda170 Compare January 8, 2025 14:13

geoand marked this pull request as ready for review January 8, 2025 14:13

michalvavrik approved these changes Jan 8, 2025

View reviewed changes

geoand requested a review from gastaldi January 8, 2025 17:20

gastaldi approved these changes Jan 8, 2025

View reviewed changes

geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 8, 2025

This comment has been minimized.

Sign in to view

Move BasicAuthenticationMechanism to synthetic bean

ba997d7

geoand force-pushed the http-mech-bean branch from 7bda170 to ba997d7 Compare January 9, 2025 14:43

quarkus-bot bot added the triage/flaky-test label Jan 9, 2025

geoand merged commit 75a1743 into quarkusio:main Jan 9, 2025
52 checks passed

quarkus-bot bot added this to the 3.18 - main milestone Jan 9, 2025

quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jan 9, 2025

geoand deleted the http-mech-bean branch January 10, 2025 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move BasicAuthenticationMechanism to synthetic bean #45442

Move BasicAuthenticationMechanism to synthetic bean #45442

geoand commented Jan 8, 2025

quarkus-bot bot commented Jan 8, 2025

michalvavrik left a comment •

edited

Loading

michalvavrik Jan 8, 2025 •

edited

Loading

geoand Jan 8, 2025

michalvavrik Jan 8, 2025

michalvavrik Jan 8, 2025 •

edited

Loading

geoand Jan 8, 2025

geoand commented Jan 8, 2025

michalvavrik commented Jan 8, 2025

geoand commented Jan 8, 2025

michalvavrik left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

quarkus-bot bot commented Jan 9, 2025 •

edited by github-actions bot

Loading

Move BasicAuthenticationMechanism to synthetic bean #45442

Move BasicAuthenticationMechanism to synthetic bean #45442

Conversation

geoand commented Jan 8, 2025

quarkus-bot bot commented Jan 8, 2025

michalvavrik left a comment • edited Loading

Choose a reason for hiding this comment

michalvavrik Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

geoand Jan 8, 2025

Choose a reason for hiding this comment

michalvavrik Jan 8, 2025

Choose a reason for hiding this comment

michalvavrik Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

geoand Jan 8, 2025

Choose a reason for hiding this comment

geoand commented Jan 8, 2025

michalvavrik commented Jan 8, 2025

geoand commented Jan 8, 2025

michalvavrik left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

quarkus-bot bot commented Jan 9, 2025 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/opentelemetry-quickstart

michalvavrik left a comment •

edited

Loading

michalvavrik Jan 8, 2025 •

edited

Loading

michalvavrik Jan 8, 2025 •

edited

Loading

quarkus-bot bot commented Jan 9, 2025 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`