Skip to content

let known queries take precedence in InMemoryPersistedQueryCache - #4489

Open
nab-iha3 wants to merge 1 commit into
graphql-java:masterfrom
nab-iha3:persisted-cache-known-queries-first
Open

nab-iha3 wants to merge 1 commit into
graphql-java:masterfrom
nab-iha3:persisted-cache-known-queries-first

Conversation

@nab-iha3

@nab-iha3 nab-iha3 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

with a query registered in InMemoryPersistedQueryCache for an id, and a request that sends that id together with its own query text:

InMemoryPersistedQueryCacheTest > known queries take precedence over the query text in the execution input
Condition not satisfied:

printAstCompact(doc) == "{foo bar baz}"
|               |    |
{somethingElse} |    false

getPersistedQueryDocumentAsync reads executionInput.getQuery() first and only falls back to knownQueries when that text is null, empty or the APQ marker. so on a cache miss the request's text is what gets compiled and stored under the id, and every later caller that sends only the id is served that document. ApolloPersistedQuerySupport is not exposed because persistedQueryIdIsInvalid rejects text whose sha256 differs from the id, but the base PersistedQuerySupport returns false there, so with any other id scheme a client can replace a server registered query by sending its own text next to a known id. the custom cache in ApolloPersistedQuerySupportTest ("will use query hash in preference to query text") already treats the registered text as authoritative; the in memory cache did the opposite.

look the id up in knownQueries first and only register text from the execution input for an id that is not known. the apollo flow is unchanged, since text that passes the hash check is the registered text anyway.

@github-actions

github-actions Bot commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Test Report

Test Results

Java Version Total Passed Failed Errors Skipped
Java 11 5956 (+1 🟢) 5900 (+1 🟢) 0 (±0) 0 (±0) 56 (±0)
Java 17 5956 (+1 🟢) 5899 (+1 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 21 5956 (+1 🟢) 5899 (+1 🟢) 0 (±0) 0 (±0) 57 (±0)
Java 25 5956 (+1 🟢) 5899 (+1 🟢) 0 (±0) 0 (±0) 57 (±0)
jcstress 32 (±0) 32 (±0) 0 (±0) 0 (±0) 0 (±0)
Total 23856 (+4 🟢) 23629 (+4 🟢) 0 (±0) 0 (±0) 227 (±0)

Code Coverage (Java 25)

Metric Covered Missed Coverage vs Master
Lines 30142 3117 90.6% ±0.0%
Branches 8837 1517 85.3% ±0.0%
Methods 8049 1207 87.0% ±0.0%

Changed Class Coverage (2 classes)

Class Line Branch Method
g.e.ExecutionStrategy ±0.0% -0.8% 🔴 ±0.0%
g.e.p.p.InMemoryPersistedQueryCache +6.3% 🟢 +13.3% 🟢 ±0.0%
ExecutionStrategy — method details
Method Line Branch
lambda$buildFieldValueMap$0 100.0% 90.0% (-10.0% 🔴)

Full HTML report: build artifact jacoco-html-report

Updated: 2026-10-02 17:19:45 UTC

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant