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

Fix JandexUtil#getBoxedTypeName() and move it to Qute extension #28536

Merged
merged 1 commit into from Oct 14, 2022

Conversation

gsmet
Copy link
Member

@gsmet gsmet commented Oct 12, 2022

Type.toString() shouldn't be used to get the type name.

Related to #28406 .

@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member Author

gsmet commented Oct 13, 2022

@mkouba I worked around the various issues but it's not pretty.

@Ladicek I wonder if we should have something in Jandex that allows to get the Type toString() without the annotations.

@mkouba
Copy link
Contributor

mkouba commented Oct 13, 2022

I did a quick search and it seems that io.quarkus.deployment.util.JandexUtil.getBoxedTypeName(Type) is only used in the QuteProcessor. resteasy-reactive has its own copy of JandexUtil (!).

So I'd recommend to deprecate the original JandexUtil.getBoxedTypeName() method and mark it for removal, move the logic to the QuteProcessor and use a more meaningful name such as getCheckedTemplateParameterTypeName(). BTW I don't think that we need the boxed value for primitives anymore because we do support template param declaration like {@int myInt}.

@Ladicek
Copy link
Contributor

Ladicek commented Oct 13, 2022

Type.toString() is just a human readable representation. We can add a human readable representation without annotations for sure. But clearly JandexUtil.getBoxedTypeName() is not meant to return a human-readable representation of the type, so it shouldn't use Type.toString(). What is it supposed to return, I don't really know, but if it's only used in Qute, I agree with @mkouba that Qute should have its own copy and this method should be deprecated for removal.

Now, Type.name() is not exactly consistently defined. For primitives (and void), it returns the corresponding Java keyword, otherwise it returns what Class.getName() would return (for parameterized types, it's the class of the raw type, and for wildcards and type variables, it's the class of the [first] upper bound). This is why it returns that weird string for arrays...

@quarkus-bot quarkus-bot bot added the area/qute The template engine label Oct 13, 2022
@gsmet
Copy link
Member Author

gsmet commented Oct 13, 2022

@mkouba see what I just pushed.

@gsmet gsmet changed the title Fix JandexUtil#getBoxedTypeName() Fix JandexUtil#getBoxedTypeName() and move it to Qute extension Oct 13, 2022
@FroMage
Copy link
Member

FroMage commented Oct 13, 2022

resteasy-reactive has its own copy of JandexUtil (!).

Well, it's supposed to be independent and move to its own repo at some point, so it can't depend on quarkus-core.

So I'd recommend to deprecate the original JandexUtil.getBoxedTypeName() method and mark it for removal, move the logic to the QuteProcessor and use a more meaningful name such as getCheckedTemplateParameterTypeName()

Why deprecate it and move it? Are you sure it will never be useful to others?

@mkouba
Copy link
Contributor

mkouba commented Oct 13, 2022

Why deprecate it and move it? Are you sure it will never be useful to others?

Well, it's not used by any core extension and it's IMO not very clear what should be the return value.

@mkouba
Copy link
Contributor

mkouba commented Oct 13, 2022

@mkouba see what I just pushed.

Looks good but I'd rather mark the method as deprecated first becuse it could be used by an extension from quarkiverse and others...

@gsmet
Copy link
Member Author

gsmet commented Oct 13, 2022

@mkouba done!

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 13, 2022

Failing Jobs - Building b8c6008

Status Name Step Failures Logs Raw logs
JVM Tests - JDK 11 Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17
✔️ JVM Tests - JDK 17 MacOS M1
JVM Tests - JDK 18 Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/opentelemetry-reactive-messaging 

📦 integration-tests/opentelemetry-reactive-messaging

io.quarkus.it.opentelemetry.OpenTelemetryTestCase.testProducerConsumerTracing line 73 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryTestCase was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ JVM Tests - JDK 18 #

- Failing: extensions/flyway/deployment 
! Skipped: integration-tests/flyway integration-tests/hibernate-orm-tenancy/datasource integration-tests/hibernate-orm-tenancy/schema and 2 more

📦 extensions/flyway/deployment

io.quarkus.flyway.test.FlywayExtensionInitSqlTest. - More details - Source on GitHub

java.lang.RuntimeException: java.lang.RuntimeException: Failed to start quarkus
	at io.quarkus.test.QuarkusUnitTest.beforeAll(QuarkusUnitTest.java:689)
	at org.junit.jupiter.engine.descriptor.ClassBasedTestDescriptor.lambda$invokeBeforeAllCallbacks$12(ClassBasedTestDescriptor.java:395)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/qute The template engine
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants