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

Provide a way to customize GraphQL ExecutionId #4877

Merged
merged 9 commits into from
May 30, 2023

Conversation

tomatophobia
Copy link
Contributor

Motivation:

  • Currently, if an ExecutionId is not explicitly set for a query in GraphQL-Java, a default ExecutionId is generated using UUID.randomUUID(). However, this default approach has drawbacks. To address these limitations, this PR introduces the ability for users to customize the generation of ExecutionId.

Modifications:

  • Introduce an ExecutionIdGenerator to enable customization of the execution ID generation mechanism using the ServiceRequestContext, query, and operation name.
    • The of() method in the ExecutionIdGenerator interface provides a default implementation that automatically assigns the execution ID as the ID of the ServiceRequestContext, allowing for convenient generation of execution IDs.
  • The method executionIdGenerator(), is introduced in the GraphqlServiceBuilder, which enables users to inject a customized ExecutionIdGenerator instance into the builder.

Result:

@tomatophobia tomatophobia changed the title Provide ExecutionIdGenerator Provide a way to customize GraphQL ExecutionId May 16, 2023
@ikhoon ikhoon added this to the 1.24.0 milestone May 17, 2023
@tomatophobia tomatophobia force-pushed the customize-graphql-execution-id branch from e51020a to f71b3fc Compare May 18, 2023 14:35
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, @tomatophobia! 👍

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.

Left some minor comments but basically looks good to me 👍 Thanks @tomatophobia ! 🙇 👍 🙇

Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

LGTM once @jrhee17's comments are addressed.
Thanks!

@tomatophobia tomatophobia force-pushed the customize-graphql-execution-id branch from 2798ea9 to 55c7db7 Compare May 26, 2023 17:10
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Nice work, @tomatophobia! ❤️

@trustin trustin merged commit 25b685c into line:main May 30, 2023
ikhoon added a commit that referenced this pull request Jun 7, 2023
Motivation:

This PR fixes `shadedTest` task to run with JUnit 5 and the bugs that weren't caught by CI builds because of the aforementioned bug.

- `shadedTest`:
  `shadedTest` was configured to run test suites with JUnit 4. It caused CI builds to pass successfully, although they failed with `gradle test` when I run locally.
  While upgrading the Gradle version to 8, we had to use JVM test suites to use both JUnit and TestNG. In Gradle 8, `Test` didn't allow overring test frameworks. For that reason, `test` was migrated to test suites but `shadedTest` hasn't changed.

- `RequestLog.resposneCause()`:
   Recently, I've encountered the following test failure repeatedly when I was running tests locally.
  ```java
  GrpcClientTest > errorNoMessage() FAILED
    java.lang.AssertionError:
    Expecting actual not to be null
        at com.linecorp.armeria.client.grpc.GrpcClientTest.lambda$errorNoMessage$44(GrpcClientTest.java:1639)
        at com.linecorp.armeria.client.grpc.GrpcClientTest.checkRequestLogError(GrpcClientTest.java:1863)
        at com.linecorp.armeria.client.grpc.GrpcClientTest.errorNoMessage(GrpcClientTest.java:1634)
  ```
  That regression was caused by the change in #3904 that added `responseCause(Throwable)` and migrated `RpcResponse.cause()` to use the new API. https://github.com/line/armeria/pull/3904/files#diff-91096a8a0f87541eb2debf804754395c3000432ccc7af9167fd99d93e4241628
  Since `RequestLog.responseCause(Throwable)` does not allow setting an exception if an HTTP response has ended, `RpcResponse.cause()`, produced after the HTTP response ended, wasn't recode to `RequestLog`.

- `RequestLog.whenComplete()`:
  There is a case where `RequestLog.whenComplete()` is never complete when `RequestLog.whenComplete()` and `RequestLogBuilder.endResponse()` are invoked simultaneously.
- GraphQL's `ExcutionId`:
  An `ExcutionIdGenerator` can be set to `GraphQL` while building a service. But the obsolete `ExcutionId` set to `ExecutionInput` wasn't removed. #4877 

- `%3F`:
  - `RequestTarget` is fixed not to decode `%3F` into `?` in a path component but some tests weren't fixed. #4907

- Test Java version:
  `-PtestJavaVersion` wasn't properly set to non-`test` tasks such as `shadedTest`, `testNG`, `testStreaming`, and so on.

Note:

The regression has not yet been released, so there is no impact on users.

Modifications:

- Migrated `shadedTest` to JVM test suits to run tests with JUnit 5.
- Fixed broken tests caused by #4907
- Directly set `RpcResponse.cause()` to `responseCause` if it is null
- Read `flags` within `lock` before removing satisfied futures in `DefaultRequestLog`

Result:

Make CI builds run all tests
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.

Provide a way to customize GraphQL ExecutionId
5 participants