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

Detect @Tenant interceptors are required when only JAX-RS classes are annotated, but no resource methods #40845

Conversation

michalvavrik
Copy link
Member

Copy link

quarkus-bot bot commented May 26, 2024

Status for workflow Quarkus CI

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

✅ 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.

@sberyozkin
Copy link
Member

sberyozkin commented May 26, 2024

@michalvavrik Michal, why don't we want to use these interceptors on methods ? Like

class Login {
    @Tenant("google") 
    @GET
    public Response loginWithGoogle() {}

    @Tenant("github") 
    @GET
    public Response loginWithGithub() {}
}

Or did you mean something else ?

@sberyozkin
Copy link
Member

Is it about @Inject @Tenant("google") TenanIdentityProvider provider; ?

@michalvavrik
Copy link
Member Author

Sorry @sberyozkin I wasn't clear. Your example will work as well, we have tests.

I made a mistake in #40843 that I only realized when re-reading it today. On JAX-RS resources, you can place @Tenant("hr") like this:

@Tenant("github") 
class Login {

    @GET
    public Response loginWithGoogle() {}

    @GET
    public Response loginWithGithub() {}
}

that is working, we have tests which https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantEchoResource.java#L16 part of. However when we try to determine whether tenant interceptors should be produced, I tried to exclude TenantIdentityProvider and I forgot to check for the Tenant annotation instances on classes.

Only reason why CI is not failing is that there are other tests using @Tenant on methods here: https://github.com/quarkusio/quarkus/blob/main/integration-tests/oidc-tenancy/src/main/java/io/quarkus/it/keycloak/TenantEcho2Resource.java#L26

So this PR is about detecting that @Tenant interceptors are required when @Tenant("github") is not used on a method level.

I can create a test if you like, but as said, it is not about actual functionality, just about knowing whether functionality is needed. I'll respect your choice.

@sberyozkin
Copy link
Member

@michalvavrik Got it, thanks

@sberyozkin sberyozkin merged commit 69b8c41 into quarkusio:main May 26, 2024
22 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.12 - main milestone May 26, 2024
@michalvavrik michalvavrik deleted the feature/tweak-tenant-ann-detection-on-classes branch May 26, 2024 18:14
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