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

Improve stereotypes support #26264

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Jun 21, 2022

This commit adds a better API for registering custom stereotypes, similar
to existing APIs: StereotypeRegistrar and StereotypeRegistrarBuildItem.
The existing AdditionalStereotypeBuildItem is very confusing to use, so
it is deprecated and all its usages are replaced with the new API.

Also, this commit allows defining stereotypes for synthetic beans. Scope,
name, alternative status and priority are all applied to the synthetic bean,
but interceptor bindings are not. We don't apply interceptors to synthetic
beans in general, this case is not an exception.

@Ladicek Ladicek added the area/arc Issue related to ARC (dependency injection) label Jun 21, 2022
@Ladicek Ladicek requested a review from mkouba June 21, 2022 15:53
@Ladicek
Copy link
Contributor Author

Ladicek commented Jun 21, 2022

Creating as draft to let CI run in my fork, and to discuss the TODO items with @mkouba.

@Ladicek Ladicek force-pushed the stereotype-improvements branch from 57cadcd to 26a5640 Compare June 22, 2022 08:11
@quarkus-bot quarkus-bot bot added area/documentation area/health area/smallrye area/spring Issues relating to the Spring integration labels Jun 22, 2022
@Ladicek Ladicek force-pushed the stereotype-improvements branch from 26a5640 to 8e6b569 Compare July 28, 2022 11:55
@Ladicek
Copy link
Contributor Author

Ladicek commented Jul 28, 2022

Rebased. @mkouba could you please take a look? Thank you!

@Ladicek Ladicek force-pushed the stereotype-improvements branch from 8e6b569 to 5419afd Compare August 16, 2022 11:38
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 16, 2022

Removed all TODOs, so marking as ready.

@Ladicek Ladicek marked this pull request as ready for review August 16, 2022 11:38
@quarkus-bot

This comment has been minimized.

@Ladicek Ladicek force-pushed the stereotype-improvements branch from 5419afd to a010610 Compare August 17, 2022 12:17
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 17, 2022

Rebased to get rid of the RestClient test failures.

@quarkus-bot

This comment has been minimized.

@mkouba
Copy link
Contributor

mkouba commented Aug 19, 2022

The CI failure may be related?

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 19, 2022

Yea I need to check why the SmallRye Health test failed in CI, it all passed locally.

@mkouba
Copy link
Contributor

mkouba commented Aug 30, 2022

Yea I need to check why the SmallRye Health test failed in CI, it all passed locally.

So what's the status of this PR?

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2022

I'm just getting back to this. (I wanted to work on #27574 and figured it would be best to have this done first).

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2022

I can also just get rid of the SmallRye Health changes, as they are not really relevant to the subject of this PR. I just stumbled upon that piece of code and found it's suboptimal, so I figured I could just fix it and squeeze it in. I'll investigate for a bit and if I don't manage to find the issue, I'll get rid of the changes here.

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2022

So the test failure from CI is:

Expected: iterable with items ["alpha1", "bravo1"] in any order
  Actual: <[alpha2, bravo1]>

The health checks are:

static class HealthCheckProducers {
    static final AtomicInteger ALPHA_COUNTER = new AtomicInteger();
    static final AtomicInteger BRAVO_COUNTER = new AtomicInteger();

    // No scope - @Singleton is used by default
    @Readiness
    HealthCheck alpha() {
        int idx = ALPHA_COUNTER.incrementAndGet();
        return () -> HealthCheckResponse.builder().up().name("alpha" + idx).build();
    }

    @RequestScoped
    @Readiness
    HealthCheck bravo() {
        int idx = BRAVO_COUNTER.incrementAndGet();
        return () -> HealthCheckResponse.builder().up().name("bravo" + idx).build();
    }
}

So alpha has no scope annotation, thereby defaulting to singleton. There should only ever be alpha1. The existence of alpha2 means the alpha health check has a different scope. I have no idea why.

Gonna revert those changes, then.

@Ladicek Ladicek force-pushed the stereotype-improvements branch from a010610 to c13b017 Compare August 30, 2022 14:21
@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2022

Done.

@mkouba
Copy link
Contributor

mkouba commented Aug 30, 2022

So alpha has no scope annotation, thereby defaulting to singleton. There should only ever be alpha1. The existence of alpha2 means the alpha health check has a different scope. I have no idea why.

I believe that this would deserve a little investigation. Do you have a diff somewhere or a branch with a state before you reversed the change?

@Ladicek
Copy link
Contributor Author

Ladicek commented Aug 30, 2022

I do: https://github.com/Ladicek/quarkus/commits/smallrye-health-default-scope The interesting thing is, the test didn't fail for me locally, and only failed in CI on Windows. I don't know what to take from it.

@mkouba
Copy link
Contributor

mkouba commented Aug 30, 2022

The interesting thing is, the test didn't fail for me locally, and only failed in CI on Windows. I don't know what to take from it.

That's superweird.

@quarkus-bot

This comment has been minimized.

This commit adds a better API for registering custom stereotypes, similar
to existing APIs: `StereotypeRegistrar` and `StereotypeRegistrarBuildItem`.
The existing `AdditionalStereotypeBuildItem` is very confusing to use, so
it is deprecated and all its usages are replaced with the new API.

Also, this commit allows defining stereotypes for synthetic beans. Scope,
name, alternative status and priority are all applied to the synthetic bean,
but interceptor bindings are not. We don't apply interceptors to synthetic
beans in general, this case is not an exception.
@mkouba mkouba force-pushed the stereotype-improvements branch from c13b017 to 320681c Compare August 31, 2022 07:37
@mkouba mkouba added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 31, 2022
@mkouba mkouba merged commit 56e0b56 into quarkusio:main Aug 31, 2022
@quarkus-bot quarkus-bot bot added this to the 2.13 - main milestone Aug 31, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Aug 31, 2022
@Ladicek Ladicek deleted the stereotype-improvements branch August 31, 2022 15:14
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/documentation area/health area/smallrye area/spring Issues relating to the Spring integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants