-
Notifications
You must be signed in to change notification settings - Fork 273
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
[FOUN-575] persisted queries have client names #6198
base: dev
Are you sure you want to change the base?
Conversation
This depends on unreleased functionality of GraphOS.
✅ Docs Preview ReadyNo new or changed pages found. |
@glasser, please consider creating a changeset entry in |
CI performance tests
|
{ | ||
if let Some(persisted_query_body) = manifest_poller.get_operation_body( | ||
persisted_query_id, | ||
request.context.get(CLIENT_NAME).unwrap_or_default(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably implies that the new feature only works if Apollo telemetry is enabled. This is probably a reasonable constraint for an initial release, but we should at least document it.
.cloned() | ||
{ | ||
Some(body) | ||
} else if client_name.is_some() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this else if? It looks like we would be returning the null operation for the a request for an operation with a client_name. If client_name
is Some and nothing exists in state.persisted_query_manifest
for it, None
feels like the right return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this is saying is — if we don't have an operation that is specifically designated for this client name, try one that was published without a client name. (If we didn't have this, all PQs would suddenly stop working for any clients that send a client name as soon as you upgraded to this version.)
The reason it's a conditional at all instead of just an "else try client_name: None
" is because if client_name
is None, it would be making the same exact call twice, which seems wasteful.
I should add comments to this function though!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooohhhh riiight! that totally makes sense! comment will definitely help, but logic is definitely good! thanks :)
This depends on unreleased functionality of GraphOS.