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

Properly sort beans in DevUI beans page #29737

Merged
merged 1 commit into from Dec 9, 2022
Merged

Conversation

geoand
Copy link
Contributor

@geoand geoand commented Dec 7, 2022

Fixes: #29723

@geoand geoand requested a review from mkouba December 7, 2022 14:40
@quarkus-bot quarkus-bot bot added the area/arc Issue related to ARC (dependency injection) label Dec 7, 2022
@@ -101,6 +101,11 @@ static String createSimple(AnnotationInstance annotation) {

@Override
public int compareTo(Name other) {
// Quarkus classes should be last
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really needed? I don't see a benefit of this change. I mean why should e.g. the io.smallrye.config.inject.ConfigProducer go before io.quarkus.vertx.http.runtime.CurrentVertxRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is needed because you can have application beans that are generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

But generated application beans are handled in the DevBeanInfo#compare(), or?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point is that you can have beans that are both application beans and generated beans and some come from Quarkus (with no user action).
So by doing this the Quarkus ones are moved to the bottom

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I don't think that ordering matters here because users are probably not interested in generated beans anyway (be it a quarkus class or not).

In other words, this should be enough (and already quite confusing ;-):

  1. application beans, non-generated, sorted by providerType name
  2. application beans, generated, sorted by providerType name
  3. non-app beans, non-generated, sorted by providerType name
  4. non-app beans, generated, sorted by providerType name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

io.quarkus.resteasy.reactive.server.runtime.NotFoundExceptionMapper$GeneratedExceptionHandlerFor$NotFoundException$OfMethod$toResponse, io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.AuthenticationFailedExceptionMapper$GeneratedExceptionHandlerFor$AuthenticationFailedException$OfMethod$handle among a few others.

These are always included with RESTEasy Reactive

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway, it's not a blocker so I'm going to approve this pr ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

io.quarkus.resteasy.reactive.server.runtime.NotFoundExceptionMapper$GeneratedExceptionHandlerFor$NotFoundException$OfMethod$toResponse, io.quarkus.resteasy.reactive.server.runtime.exceptionmappers.AuthenticationFailedExceptionMapper$GeneratedExceptionHandlerFor$AuthenticationFailedException$OfMethod$handle among a few others.

These are always included with RESTEasy Reactive

Yes, but these are generated and should go after non-generated app beans...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's my point. Without this change, they are displayed before the generated beans that are a result of the user writing some code.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering what those "non-quarkus application generated beans" are...

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@quarkus-bot
Copy link

quarkus-bot bot commented Dec 8, 2022

Failing Jobs - Building 4667cd7

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
JVM Tests - JDK 17 MacOS M1 Set up runner ⚠️ Check → Logs Raw logs
✔️ JVM Tests - JDK 18

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/vertx/deployment 
! Skipped: extensions/agroal/deployment extensions/amazon-lambda-http/deployment extensions/amazon-lambda-rest/deployment and 341 more

📦 extensions/vertx/deployment

io.quarkus.vertx.mdc.VertxMDCTest.mdc line 144 - More details - Source on GitHub

org.opentest4j.AssertionFailedError: expected: <10> but was: <0>
	at org.junit.jupiter.api.AssertionFailureBuilder.build(AssertionFailureBuilder.java:151)
	at org.junit.jupiter.api.AssertionFailureBuilder.buildAndThrow(AssertionFailureBuilder.java:132)

io.quarkus.vertx.mdc.VertxMDCTest.mdcNonVertxThreadTest line 121 - More details - Source on GitHub

java.lang.AssertionError: 

Expected: a collection containing "Test MDC value ### Test 1"

@geoand geoand merged commit a8ae835 into quarkusio:main Dec 9, 2022
@geoand geoand deleted the #29723 branch December 9, 2022 08:27
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 9, 2022
@gsmet gsmet modified the milestones: 2.16 - main, 2.15.1.Final Dec 20, 2022
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) kind/bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Application beans are not shown first in Arc DevUI
3 participants