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.

Sign up for GitHub

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

Jump to bottom

fix(vertx): do not log a warning about missing json support when vertx extension is present #5623

Merged
merged 1 commit into from
Nov 20, 2019
Merged

fix(vertx): do not log a warning about missing json support when vertx extension is present #5623

merged 1 commit into from
Nov 20, 2019

Conversation

machi1990
Copy link
Member

Vert.x also provides a JSON implementation so no warning should be emitted

Fixes #5614

/cc @geoand @gsmet

@machi1990 machi1990 requested review from gsmet and geoand November 20, 2019 12:17
@machi1990 machi1990 added the area/user-experience Will make us lose users label Nov 20, 2019
@machi1990 machi1990 added this to the 1.1.0 milestone Nov 20, 2019
gsmet
gsmet reviewed Nov 20, 2019
View reviewed changes
extensions/vertx/deployment/src/main/java/io/quarkus/vertx/deployment/VertxProcessor.java
@@ -51,6 +52,11 @@
@Inject
BuildProducer<ReflectiveClassBuildItem> reflectiveClass;

@BuildStep(providesCapabilities = Capabilities.RESTEASY_JSON_EXTENSION)
Copy link
Member

Choose a reason for hiding this comment

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

It's not used for anything else?

Copy link
Contributor

Choose a reason for hiding this comment

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

Which part do you mean? Sorry for my thick headedness :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The RESTEASY_JSON_EXTENSION Capability is used in jackson and jsonb extension to indicate json be support in ResteasyCommonProcessor. It was added for that case IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it already exists which is I guess what @gsmet was asking :)

Copy link
Member

Choose a reason for hiding this comment

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

Well, I'm wondering if it's a good idea to reuse that one or if we should add a VERTX capability and alter the condition in the processor itself.

This capability was used to basically say "we have an extension to provide proper JSON support for RESTEasy" and this one is not exactly saying that. It's saying "Vert.x provides some sort of JSON support for its own types".

Maybe I'm overthinking it, I don't know.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we are OK with that @machi1990 added for the time being. My view is that if we need something more complicated later, we can always improve things

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 @geoand

WDYT @gsmet ?

Copy link
Member

Choose a reason for hiding this comment

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

I think I agree with myself but let's do it your way and I'll keep my "I told you so" for later :).

Copy link
Contributor

Choose a reason for hiding this comment

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

It wouldn't be the first time 😁

Copy link
Member Author

Choose a reason for hiding this comment

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

It wouldn't be the first time 😁

ha ha indeed :-)

@geoand
Copy link
Contributor

geoand commented Nov 20, 2019

It looks good to me, but I'll refrain from approving since I @gsmet to clarify what he meant in his question

@machi1990
Copy link
Member Author

CI failure unrelated

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.8.0-jboss-2:compile (default-compile) on project quarkus-integration-test-hibernate-orm-panache: Compilation failure: Compilation failure: 
[ERROR] /home/vsts/work/1/s/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java:[91,51] cannot find symbol
[ERROR]   symbol:   method withLock(javax.persistence.LockModeType)
[ERROR]   location: interface io.quarkus.hibernate.orm.panache.PanacheQuery<io.quarkus.hibernate.orm.panache.PanacheEntityBase>
[ERROR] /home/vsts/work/1/s/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java:[141,22] method findById in class io.quarkus.hibernate.orm.panache.PanacheEntityBase cannot be applied to given types;
[ERROR]   required: java.lang.Object
[ERROR]   found: java.lang.Long,javax.persistence.LockModeType
[ERROR]   reason: cannot infer type-variable(s) T
[ERROR]     (actual and formal argument lists differ in length)
[ERROR] /home/vsts/work/1/s/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java:[371,54] cannot find symbol
[ERROR]   symbol:   method withLock(javax.persistence.LockModeType)
[ERROR]   location: interface io.quarkus.hibernate.orm.panache.PanacheQuery<io.quarkus.it.panache.Person>
[ERROR] /home/vsts/work/1/s/integration-tests/hibernate-orm-panache/src/main/java/io/quarkus/it/panache/TestEndpoint.java:[420,25] method findById in interface io.quarkus.hibernate.orm.panache.PanacheRepositoryBase<Entity,Id> cannot be applied to given types;
[ERROR]   required: java.lang.Long
[ERROR]   found: java.lang.Long,javax.persistence.LockModeType
[ERROR]   reason: actual and formal argument lists differ in length
[ERROR] -> [Help 1]

@geoand
Copy link
Contributor

geoand commented Nov 20, 2019

@loicmathieu does the error ring a bell? Is there perhaps a flaky test?

@loicmathieu
Copy link
Contributor

@geoand it has been introduced via #5583 and all test passed on this one.
And tests passed on JDK11 and JKD12 so ... this is very strange.

I advise to re-launch the CI

@geoand
Copy link
Contributor

geoand commented Nov 20, 2019 •
edited
Loading

I will restart. Just please keep it in mind that the test might be flaky and we might need a fix

@loicmathieu
Copy link
Contributor

I will test. Just please keep it in mind that the test might be flaky and we might need a fix

Hum, this will be very surprising, it compiles or not ... and I kust compiled it with Java 8 without any error ...

@geoand
Copy link
Contributor

geoand commented Nov 20, 2019

Oh OK, I might know the cause.

@machi1990 can you please rebase onto the latest master?

@machi1990
Copy link
Member Author

I think it is more a cache thingy on the CI @geoand @loicmathieu I'll rebase with master

@machi1990
fix(vertx): do not log a warning about missing json support when vert…
ab682b2 
…x extension is present

Vert.x also provides a JSON implementation so no warning should be emitted

Fixes quarkusio#5614
@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 20, 2019
@gsmet gsmet merged commit 5a8f6d8 into quarkusio:master Nov 20, 2019
@machi1990 machi1990 deleted the fix/5614 branch November 20, 2019 20:37
@gsmet gsmet removed the backport? label Nov 22, 2019
@gsmet gsmet modified the milestones: 1.1.0, 1.0.0.Final Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gsmet gsmet gsmet approved these changes

@geoand geoand Awaiting requested review from geoand

Assignees
No one assigned
Labels
area/user-experience Will make us lose users triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.0.0.Final
Development

Successfully merging this pull request may close these issues.

Vert.x also provides a JSON implementation so no warning should be emitted
4 participants
@machi1990 @geoand @loicmathieu @gsmet