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

feat: generate operation names for subgraph fetches #1550

Merged
merged 1 commit into from
Mar 3, 2022
Merged

feat: generate operation names for subgraph fetches #1550

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 32 additions & 16 deletions gateway-js/src/__tests__/build-query-plan.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1009,7 +1009,8 @@ Scenario: supports basic, single-service mutation
"password"
],
"operationKind": "mutation",
"operation": "mutation($username:String!$password:String!){login(username:$username password:$password){id}}"
"operation": "mutation Login_accounts_0($username:String!$password:String!){login(username:$username password:$password){id}}",
"operationName": "Login_accounts_0"
}
}
"""
Expand Down Expand Up @@ -1043,7 +1044,8 @@ Scenario: supports mutations with a cross-service request
"password"
],
"operationKind": "mutation",
"operation": "mutation($username:String!$password:String!){login(username:$username password:$password){__typename id}}"
"operation": "mutation Login_accounts_0($username:String!$password:String!){login(username:$username password:$password){__typename id}}",
"operationName": "Login_accounts_0"
},
{
"kind": "Flatten",
Expand Down Expand Up @@ -1071,7 +1073,8 @@ Scenario: supports mutations with a cross-service request
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}"
"operation": "query Login_reviews_1($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}",
"operationName": "Login_reviews_1"
}
},
{
Expand Down Expand Up @@ -1103,7 +1106,8 @@ Scenario: supports mutations with a cross-service request
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}"
"operation": "query Login_product_2($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}",
"operationName": "Login_product_2"
}
}
]
Expand Down Expand Up @@ -1138,7 +1142,8 @@ Scenario: returning across service boundaries
"body"
],
"operationKind": "mutation",
"operation": "mutation($upc:String!$body:String!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{__typename upc}}}"
"operation": "mutation Review_reviews_0($upc:String!$body:String!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{__typename upc}}}",
"operationName": "Review_reviews_0"
},
{
"kind": "Flatten",
Expand Down Expand Up @@ -1166,7 +1171,8 @@ Scenario: returning across service boundaries
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on Furniture{name}}}"
"operation": "query Review_product_1($representations:[_Any!]!){_entities(representations:$representations){...on Furniture{name}}}",
"operationName": "Review_product_1"
}
}
]
Expand Down Expand Up @@ -1213,7 +1219,8 @@ Scenario: supports multiple root mutations
"password"
],
"operationKind": "mutation",
"operation": "mutation($username:String!$password:String!){login(username:$username password:$password){__typename id}}"
"operation": "mutation LoginAndReview_accounts_0($username:String!$password:String!){login(username:$username password:$password){__typename id}}",
"operationName": "LoginAndReview_accounts_0"
Copy link
Member

@abernix abernix Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Very thankful you chose to add operationName in here! (In a general sense, not just on this snapshot in particular!)

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 had to in order to provide the operationName query param on the request, because that's probably what observability tools look at (instead of parsing the operation doc).

Copy link
Member

Choose a reason for hiding this comment

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

For context as to why i'm glad: it's only recently that we started adding operationKind in there, which enabled the router to make Good decisions without needing to — in the same spirit as you're describing — re-parse the operation.

},
{
"kind": "Flatten",
Expand Down Expand Up @@ -1241,7 +1248,8 @@ Scenario: supports multiple root mutations
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}"
"operation": "query LoginAndReview_reviews_1($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}",
"operationName": "LoginAndReview_reviews_1"
}
},
{
Expand Down Expand Up @@ -1273,7 +1281,8 @@ Scenario: supports multiple root mutations
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}"
"operation": "query LoginAndReview_product_2($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}",
"operationName": "LoginAndReview_product_2"
}
},
{
Expand All @@ -1284,7 +1293,8 @@ Scenario: supports multiple root mutations
"body"
],
"operationKind": "mutation",
"operation": "mutation($upc:String!$body:String!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{__typename upc}}}"
"operation": "mutation LoginAndReview_reviews_3($upc:String!$body:String!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{__typename upc}}}",
"operationName": "LoginAndReview_reviews_3"
},
{
"kind": "Flatten",
Expand Down Expand Up @@ -1312,7 +1322,8 @@ Scenario: supports multiple root mutations
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on Furniture{name}}}"
"operation": "query LoginAndReview_product_4($representations:[_Any!]!){_entities(representations:$representations){...on Furniture{name}}}",
"operationName": "LoginAndReview_product_4"
}
}
]
Expand Down Expand Up @@ -1367,7 +1378,8 @@ Scenario: multiple root mutations with correct service order
"updatedReview"
],
"operationKind": "mutation",
"operation": "mutation($upc:String!$body:String!$updatedReview:UpdateReviewInput!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{upc}}updateReview(review:$updatedReview){id body}}"
"operation": "mutation LoginAndReview_reviews_0($upc:String!$body:String!$updatedReview:UpdateReviewInput!){reviewProduct(input:{upc:$upc body:$body}){__typename ...on Furniture{upc}}updateReview(review:$updatedReview){id body}}",
"operationName": "LoginAndReview_reviews_0"
},
{
"kind": "Fetch",
Expand All @@ -1377,7 +1389,8 @@ Scenario: multiple root mutations with correct service order
"password"
],
"operationKind": "mutation",
"operation": "mutation($username:String!$password:String!){login(username:$username password:$password){__typename id}}"
"operation": "mutation LoginAndReview_accounts_1($username:String!$password:String!){login(username:$username password:$password){__typename id}}",
"operationName": "LoginAndReview_accounts_1"
},
{
"kind": "Flatten",
Expand Down Expand Up @@ -1405,7 +1418,8 @@ Scenario: multiple root mutations with correct service order
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}"
"operation": "query LoginAndReview_reviews_2($representations:[_Any!]!){_entities(representations:$representations){...on User{reviews{product{__typename ...on Book{__typename isbn}...on Furniture{upc}}}}}}",
"operationName": "LoginAndReview_reviews_2"
Copy link
Member

@abernix abernix Mar 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@pcmanus @lennyburdette Picking a random place to leave this post-merge thought — would it make sense to use double-underscores as the delimiter to create a more predictable pattern for parsing and to avoid name-conflicts across operation + subgraph names? Mostly just because it seems not completely unlikely that both an operation and a subgraph might use _ on its own and, while not forbidden, much less likely that someone would use __. We could even forbid __ as a valid graph name (and probably should or may already have a enforced character set somewhere?).

Just trying to avoid the case of two things end up being exactly equal. If my service name is movie_titles and my operation is get_movie_titles, I would have get_movie_titles_movie_titles_0. It would be nice if I could split on something that's: forbidden in the service name and therefore reliably delineates parts, and get_movie_titles__movie_titles__0 would get that.

It was similarly intentional that I'd suggested (in addition to the above) prefixing with __ in the (Slack) thread where this initially came up — just to indicate the generated-ness and avoid these name clashes, just in case people use these operations downstream in meaningful ways.

Also happy to ticket this for follow-up. Or not, depending on thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no strong feelings either way. i went with fewer characters but four extra underscores (two in the operationName query param and two in the operation document) isn't a huge deal.

Copy link
Member

Choose a reason for hiding this comment

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

I also like the idea of the double underscore separator, and if we are not already we should enforce that subgraphs have to start and end with a number or letter

}
},
{
Expand Down Expand Up @@ -1437,7 +1451,8 @@ Scenario: multiple root mutations with correct service order
],
"variableUsages": [],
"operationKind": "query",
"operation": "query($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}"
"operation": "query LoginAndReview_product_3($representations:[_Any!]!){_entities(representations:$representations){...on Book{upc}}}",
"operationName": "LoginAndReview_product_3"
}
},
{
Expand All @@ -1447,7 +1462,8 @@ Scenario: multiple root mutations with correct service order
"reviewId"
],
"operationKind": "mutation",
"operation": "mutation($reviewId:ID!){deleteReview(id:$reviewId)}"
"operation": "mutation LoginAndReview_reviews_4($reviewId:ID!){deleteReview(id:$reviewId)}",
"operationName": "LoginAndReview_reviews_4"
}
]
}
Expand Down
4 changes: 4 additions & 0 deletions gateway-js/src/executeQueryPlan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,7 @@ async function executeFetch<TContext>(
context,
fetch.operation,
variables,
fetch.operationName
);

for (const entity of entities) {
Expand Down Expand Up @@ -333,6 +334,7 @@ async function executeFetch<TContext>(
context,
fetch.operation,
{...variables, representations},
fetch.operationName
);

if (!dataReceivedFromService) {
Expand Down Expand Up @@ -374,6 +376,7 @@ async function executeFetch<TContext>(
context: ExecutionContext<TContext>,
source: string,
variables: Record<string, any>,
operationName: string | undefined
): Promise<ResultMap | void | null> {
// We declare this as 'any' because it is missing url and method, which
// GraphQLRequest.http is supposed to have if it exists.
Expand Down Expand Up @@ -402,6 +405,7 @@ async function executeFetch<TContext>(
request: {
query: source,
variables,
operationName,
http,
},
incomingRequestContext: context.requestContext,
Expand Down
1 change: 1 addition & 0 deletions internals-js/src/operations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1372,6 +1372,7 @@ export function operationToDocument(operation: Operation): DocumentNode {
const operationAST: OperationDefinitionNode = {
kind: Kind.OPERATION_DEFINITION,
operation: operation.rootKind as OperationTypeNode,
name: operation.name ? { kind: Kind.NAME, value: operation.name } : undefined,
selectionSet: operation.selectionSet.toSelectionSetNode(),
variableDefinitions: operation.variableDefinitions.toVariableDefinitionNodes(),
};
Expand Down
1 change: 1 addition & 0 deletions query-planner-js/src/QueryPlan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface FetchNode {
variableUsages?: string[];
requires?: QueryPlanSelectionNode[];
operation: string;
operationName: string | undefined;
operationKind: OperationTypeNode;
}

Expand Down
Loading