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

Replace deprecated methods and classes in Quarkus Security and tiny bit of refactoring #44306

Conversation

michalvavrik
Copy link
Member

Changes:

  • stop using AnnotationsTransformer constructor marked for removal
  • stop storing SecurityCheckStorage as static volatile variable on recoreder as we dont' need it there
  • replecate deprecated io.smallrye.mutiny.groups.UniAndGroupIterable#combinedWith(java.util.function.Function<java.util.List<?>,O>) as it is marked for removal
  • reduces usage of raw types in QuarkusIdentityProviderManagerImpl, it's not great, but hopefully result is bit better
  • marking io.quarkus.security.spi.AdditionalSecuredClassesBuildItem for removal, it has been deprecated since 2.15
  • NativeOrNativeSourcesBuild is deprecated, IMO we could always produce the NativeImageFeatureBuildItem and Quarkus should take care of it. Nevertheless, in order to keep original behavior, I replaced it with the quarkus.native.enabled config property

Copy link

quarkus-bot bot commented Nov 5, 2024

Status for workflow Quarkus CI

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

✅ 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

📦 extensions/resteasy-reactive/rest-client/deployment

io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService - History

  • expected: "hello, Alice" but was: "hello, I'm a slow server" - org.opentest4j.AssertionFailedError
org.opentest4j.AssertionFailedError: 

expected: "hello, Alice"
 but was: "hello, I'm a slow server"
	at io.quarkus.rest.client.reactive.stork.StorkResponseTimeLoadBalancerTest.shouldUseFasterService(StorkResponseTimeLoadBalancerTest.java:58)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)
	at io.quarkus.test.QuarkusUnitTest.runExtensionMethod(QuarkusUnitTest.java:513)
	at io.quarkus.test.QuarkusUnitTest.interceptTestMethod(QuarkusUnitTest.java:427)

@@ -101,16 +102,16 @@ public Uni<? extends SecurityIdentity> apply(SecurityIdentity securityIdentity)
* @return The first identity provider that was registered with this type
*/
public SecurityIdentity authenticateBlocking(AuthenticationRequest request) {
List<IdentityProvider> providers = this.providers.get(request.getClass());
var providers = this.providers.get(request.getClass());
Copy link
Member

Choose a reason for hiding this comment

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

To me it looks like it is becoming less obvious what the actual var type is, we'll have no idea what providers are when we look at this code in 6 months :-), I don't really mind, I'm just not sure about across the board replacement of local type references with var...

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 changed this because List<IdentityProvider> is a raw type and var is shorter to write :-) I don't give importance to things like these, for me it's like a book. I'll add it back (with a wildcard) because if it is better readable for someone like you, it should be there.

Copy link
Member

Choose a reason for hiding this comment

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

@michalvavrik It is ok, you don't have to...

Copy link
Member Author

Choose a reason for hiding this comment

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

NP, I am finishing other PR, so if this one is not merged in an hour, I'll change it. One way or another thanks.

@sberyozkin sberyozkin merged commit 92e09b9 into quarkusio:main Nov 5, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 5, 2024
@michalvavrik michalvavrik deleted the feature/security-extension-refactoring branch November 5, 2024 12:41
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.

2 participants