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

Make the vertx-graphql extension work in native mode #5253

Merged
merged 4 commits into from
Nov 29, 2019
Merged

Make the vertx-graphql extension work in native mode #5253

merged 4 commits into from
Nov 29, 2019

Conversation

machi1990
Copy link
Member

fixes #5248

/cc @rsvoboda can you check if this fix the issue for you? Thanks

@gsmet @cescoffier @cristianonicolai

@machi1990 machi1990 requested a review from rsvoboda November 6, 2019 13:21
@rsvoboda
Copy link
Member

rsvoboda commented Nov 6, 2019

What's the motivation to remove @Inject Vertx vertx;?

@machi1990
Copy link
Member Author

What's the motivation to remove @Inject Vertx vertx;?

It was throwing NPE in native mode.

@rsvoboda
Copy link
Member

rsvoboda commented Nov 6, 2019

It was throwing NPE in native mode.

@Inject is not working because it's in test ?

gsmet
gsmet requested changes Nov 6, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a few comments and questions, let's sort them out.

...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
integration-tests/vertx-graphql/src/test/java/io/quarkus/vertx/graphql/it/VertxGraphqlTest.java Show resolved Hide resolved
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 6c11fd8 to bfab88e Compare November 6, 2019 15:32
@machi1990 machi1990 requested a review from gsmet November 6, 2019 15:37
@machi1990
Copy link
Member Author

It was throwing NPE in native mode.

@Inject is not working because it's in test ?

It is working in JVM mode but not for native test. I could not dig deep to understand the issue though.

@cristianonicolai
Copy link
Contributor

@machi1990 thanks for the fix, I was looking into a similar fix. Let me know if I can help somehow.

@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from bfab88e to 6e64eae Compare November 6, 2019 22:06
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 6e64eae to 3fd3ce8 Compare November 12, 2019 01:04
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 3fd3ce8 to 22cb691 Compare November 12, 2019 06:34
@cescoffier
Copy link
Member

Do we want to have the UI in prod mode? That sounds more like a dev kind of thing.

@machi1990
Copy link
Member Author

Do we want to have the UI in prod mode? That sounds more like a dev kind of thing.

Yes, I was targeting the same behaviour in JVM and native mode. Maybe later on we can refine and have a configuration variable alwaysInclude like what we have in swagger-ui extension.

@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 22cb691 to 428851a Compare November 12, 2019 19:04
@machi1990 machi1990 requested a review from cescoffier November 12, 2019 19:04
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 428851a to c171f7c Compare November 13, 2019 06:40
@cescoffier
Copy link
Member

I would like having the input from @emmanuelbernard on having the UI in prod.
The UI is a query editor, it can be dangerous to have it in prod. At least we should have a flag to disable it.

@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from c171f7c to be5159d Compare November 13, 2019 11:34
@machi1990
Copy link
Member Author

I would like having the input from @emmanuelbernard on having the UI in prod.
The UI is a query editor, it can be dangerous to have it in prod. At least we should have a flag to disable it.

Thanks for this feedback. The UI is not enabled by default, so a user has explicitly enable it. On the other hand, having a flag quarkus.vertx-graphql.ui-enabled to control whether we include the UI resources or not could be handy. Maybe the extension could automatically enable the UI in dev mode. Is that a good way to go about it? @cescoffier @emmanuelbernard

@emmanuelbernard
Copy link
Member

Yes that makes sense.

@machi1990
Copy link
Member Author

Yes that makes sense.

Thanks, I'll give it a spin in couple of hours.

@cescoffier
Copy link
Member

ah no, @gsmet had commented on the PR too. I let him check his concerns.

@machi1990
Copy link
Member Author

ah no, @gsmet had commented on the PR too. I let him check his concerns.

Cool, I believe that @gsmet comment about the build item and a question about injection in native image to have been fixed or answered. Good to double check though before merging.

@machi1990
Copy link
Member Author

As build issues are unrelated, let's take the risk to merge it.

(and Thanks @machi1990 !)

Thanks @cescoffier

Been a pleasure :-) We fixed quite a few things along the way, once this is merged we can close #5448

@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 4f6a522 to 536024a Compare November 20, 2019 12:46
gsmet
gsmet requested changes Nov 20, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks good overall. I added a small comment about the config properties. WDYT?

...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 536024a to 92c1aeb Compare November 20, 2019 17:09
@machi1990 machi1990 requested a review from gsmet November 20, 2019 17:11
gsmet
gsmet requested changes Nov 20, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks good to me except for one last comment.

...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Show resolved Hide resolved
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch 2 times, most recently from 91d56d1 to 1f25a1b Compare November 20, 2019 22:33
@machi1990 machi1990 requested a review from gsmet November 21, 2019 07:08
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 1f25a1b to 0c80dd8 Compare November 23, 2019 10:51
@machi1990
Copy link
Member Author

Just rebased with master to pick config updates from #5387.

@gsmet gsmet changed the title make the vertx-graphql extension work in native mode Make the vertx-graphql extension work in native mode Nov 28, 2019
gsmet
gsmet requested changes Nov 28, 2019
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a few minor comments.

...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
...phql/deployment/src/main/java/io/quarkus/vertx/graphql/deployment/VertxGraphqlProcessor.java Outdated Show resolved Hide resolved
@machi1990 machi1990 force-pushed the fix/vertx-graphql-native branch from 0c80dd8 to 6451715 Compare November 28, 2019 16:23
@gsmet gsmet force-pushed the fix/vertx-graphql-native branch from 6451715 to 3db09bd Compare November 29, 2019 16:10
@gsmet
Copy link
Member

gsmet commented Nov 29, 2019

I just pushed a rebase to be extra sure :). Let's wait for CI.

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 29, 2019
@machi1990
Copy link
Member Author

I just pushed a rebase to be extra sure :). Let's wait for CI.

Thanks, I was about to do that. the CI failure was related to a compilation failure which was fixed in master :-)

@machi1990
Copy link
Member Author

machi1990 commented Nov 29, 2019 •
edited
Loading

All green :-) Merging, thanks all.

@machi1990 machi1990 merged commit fd36955 into quarkusio:master Nov 29, 2019
@machi1990 machi1990 deleted the fix/vertx-graphql-native branch November 29, 2019 18:00
@cescoffier cescoffier mentioned this pull request Nov 29, 2019
Closed
@gsmet gsmet added this to the 1.1.0 milestone Dec 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manovotn manovotn manovotn left review comments

@mkouba mkouba mkouba left review comments

@gsmet gsmet gsmet approved these changes

@rsvoboda rsvoboda rsvoboda approved these changes

@cristianonicolai cristianonicolai cristianonicolai approved these changes

@cescoffier cescoffier cescoffier approved these changes

@stuartwdouglas stuartwdouglas stuartwdouglas approved these changes

@emmanuelbernard emmanuelbernard Awaiting requested review from emmanuelbernard

Assignees
No one assigned
Labels
area/vertx triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.1.0.CR1
Development

Successfully merging this pull request may close these issues.

vertx-graphql fails in native mode - GraphQLInputDeserializer has no default (no arg) constructor
9 participants
@machi1990 @rsvoboda @cristianonicolai @cescoffier @emmanuelbernard @gsmet @stuartwdouglas @mkouba @manovotn