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

Add GraphQL setter to GraphqlServiceBuilder #5269

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

minwoox
Copy link
Contributor

@minwoox minwoox commented Oct 26, 2023

Motivation:
A user might want to set GraphQL directly to the GraphqlServiceBuilder which could be shared
between Expedia GraphQLRequestHandler and Armeria's GraphqlService.

Modification:

  • Add the setter method for GraphQL.

Result:

  • You can now set GraphQL to the GraphqlServiceBuilder.

Motivation:
A user might want to set `GraphQL` directly to the `GraphqlServiceBuilder`.

Modification:
- Add the setter method for `GraphQL`.

Result:
- You can now set `GraphQL` to the `GraphqlServiceBuilder`.
@minwoox minwoox added this to the 1.27.0 milestone Oct 26, 2023
@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: Patch coverage is 63.01370% with 27 lines in your changes are missing coverage. Please review.

Project coverage is 73.91%. Comparing base (14c5566) to head (62372a1).
Report is 13 commits behind head on main.

Current head 62372a1 differs from pull request most recent head 10db64f

Please upload reports for the commit 10db64f to get more accurate results.

Files Patch % Lines
.../armeria/server/graphql/GraphqlServiceBuilder.java 63.01% 9 Missing and 18 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #5269      +/-   ##
============================================
- Coverage     74.05%   73.91%   -0.14%     
+ Complexity    21253    20082    -1171     
============================================
  Files          1850     1728     -122     
  Lines         78600    74113    -4487     
  Branches      10032     9457     -575     
============================================
- Hits          58209    54784    -3425     
+ Misses        15686    14837     -849     
+ Partials       4705     4492     -213     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +98 to +101
// Others
private boolean useBlockingTaskExecutor;
@Nullable
private GraphqlErrorHandler errorHandler;
Copy link
Contributor

Choose a reason for hiding this comment

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

The two properties are only provided by Armeria and the majority are necessary to build GraphQL. Too many checkState() looks messy. How about moving properties used to build GraphQL to GraphqlBuilder?

GraphqlService
  .builder()
  .graphql() // Return GraphqlBuilder
    .schema(...)
    .dataLoaderRegistry(...)
    .and() // Return GraphqlServiceBuilder
  .useBlockingTaskExecutor(true);
  .bulid();


GraphqlService
  .builder()
  .graphql(graphql) // Directly inject GraphQL
  .useBlockingTaskExecutor(true);
  .bulid()

Copy link
Contributor Author

@minwoox minwoox Oct 26, 2023

Choose a reason for hiding this comment

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

I was actually thinking about it but GraphQL provides its own builder via new GraphQL.Builder().
Also, I found out that our builder, which is GraphqlServiceBuilder, does not provide all setters that the GraphQL provides. I think there's always a mismatch unless we keep up with the upstream change.
So I was rather thinking deprecating the setters:
https://github.com/line/armeria/pull/5269/files#diff-6b64ad742ab0aa687e003b83510d32495d933e5b3fd2a2236184d6f01bbfcc50R109-R110

Too many checkState() looks messy.

Yes, but we need it anyway because we can't just remove the setters after deprecating them. 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

So I was rather deprecating the setters:

I agree with this idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've deprecated all the setters. PTAL. 🙇

@minwoox minwoox modified the milestones: 1.27.0, 1.28.0 Jan 22, 2024
@minwoox minwoox modified the milestones: 1.28.0, 1.29.0 Mar 28, 2024
@github-actions github-actions bot added the Stale label Apr 28, 2024
@minwoox minwoox modified the milestones: 1.29.0, 1.30.0 May 21, 2024
@github-actions github-actions bot removed the Stale label May 22, 2024
@minwoox minwoox modified the milestones: 1.30.0, 1.29.0 May 23, 2024
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks @minwoox ! 🙇 👍 🙇

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up! 🙇‍♂️🙇‍♂️

@minwoox minwoox merged commit 7f2aa00 into line:main Jun 3, 2024
15 checks passed
@minwoox minwoox deleted the graphql_builder branch June 3, 2024 01:03
Dogacel pushed a commit to Dogacel/armeria that referenced this pull request Jun 8, 2024
Motivation:
A user might want to set `GraphQL` directly to the `GraphqlServiceBuilder` which could be shared
between Expedia `GraphQLRequestHandler` and Armeria's `GraphqlService`.

Modification:
- Add the setter method for `GraphQL`.

Result:
- You can now set `GraphQL` to the `GraphqlServiceBuilder`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants