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.

Sign up for GitHub

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

Jump to bottom

Sort identity providers by priority in QuarkusIdentityProviderManagerImpl #42179

Merged
merged 1 commit into from
Aug 5, 2024
Merged

Sort identity providers by priority in QuarkusIdentityProviderManagerImpl #42179

merged 1 commit into from
Aug 5, 2024

Conversation

danilopiazza
Copy link
Contributor

@danilopiazza danilopiazza commented Jul 27, 2024 •
edited by geoand
Loading

This adds a new unit test to check that the default implementation of IdentityProviderManager effectively sorts by priority() (in reverse order) a list of IdentityProviders with the same AuthenticationRequest type.

@danilopiazza
Copy link
Contributor Author

I checked for regression with:

./mvnw -Dquickly
./mvnw -f extensions/security clean install
./mvnw -f integration-tests/micrometer-security clean install
./mvnw -f integration-tests/micrometer-security clean install -Dnative

geoand
geoand reviewed Aug 1, 2024
View reviewed changes
...ty/runtime/src/main/java/io/quarkus/security/runtime/QuarkusIdentityProviderManagerImpl.java Outdated
Comment on lines 227 to 232
providers.values().forEach(providers -> providers.sort(new Comparator<IdentityProvider>() {
@Override
public int compare(IdentityProvider o1, IdentityProvider o2) {
return Integer.compare(o2.priority(), o1.priority());
}
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use lambdas here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, thank you!

I opened a new PR (#42306) to add this to CONTRIBUTING.md.

@danilopiazza danilopiazza force-pushed the identity-providers-sort-by-priority branch from c55a634 to c743eca Compare August 3, 2024 16:20
@danilopiazza danilopiazza mentioned this pull request Aug 3, 2024
Merged
geoand
geoand approved these changes Aug 5, 2024
View reviewed changes
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!

@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Aug 5, 2024 •
edited by github-actions bot
Loading

Status for workflow Quarkus CI

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

✅ 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

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.MetricsTest.directCounterTest - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds.
	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.MetricsTest.directCounterTest(MetricsTest.java:54)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

✖ io.quarkus.it.opentelemetry.MetricsTest.directCounterTest - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.MetricsTest was not fulfilled within 5 seconds.
	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.MetricsTest.directCounterTest(MetricsTest.java:54)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@geoand geoand merged commit 09f6957 into quarkusio:main Aug 5, 2024
32 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/security kind/bugfix triage/flaky-test
Projects
None yet
Milestone
3.14.0.CR1
Development

Successfully merging this pull request may close these issues.

Sort identity providers by priority in QuarkusIdentityProviderManagerImpl
2 participants
@danilopiazza @geoand