feat(gate): idempotent request #1031
Conversation
📝 Walkthrough""" WalkthroughThis change removes a specific port and its metadata from the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLService
participant RegisterCache
Client->>GraphQLService: Send GraphQL request (with/without Idempotency-Key)
alt Idempotency-Key present
GraphQLService->>RegisterCache: getResponse(key)
alt Cached response found and not expired
GraphQLService->>GraphQLService: computeRequestSignature(request)
alt Signatures match
RegisterCache-->>GraphQLService: Cached response
GraphQLService-->>Client: Return cached response
else Signatures differ
GraphQLService-->>Client: Return 422 error (request shape changed)
end
else Cached response expired or missing
GraphQLService->>GraphQLService: handleGraphQLHelper(operations)
GraphQLService->>RegisterCache: addResponse(key, response, expiry, signature)
GraphQLService-->>Client: Return new response
end
else No Idempotency-Key
GraphQLService->>GraphQLService: handleGraphQLHelper(operations)
GraphQLService-->>Client: Return new response
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/typegate/src/services/graphql_service.ts (2)
179-185: Expiry clock starts before query executes
nowis captured before the potentially long-running call tohandleGraphQLHelper, effectively shortening TTL by the query latency.
Moveconst now = Date.now()right before computingexpiryMillis.
154-191: Key uniqueness insufficient – collisions across routes & methodsOnly the raw header value is used; two different endpoints (or methods) sharing a key will collide.
Consider composing the key with HTTP method + path (and maybe hashed payload) to guarantee uniqueness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.ghjk/lock.json(0 hunks)src/typegate/src/services/graphql_service.ts(3 hunks)
💤 Files with no reviewable changes (1)
- .ghjk/lock.json
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: pre-commit
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/typegate/src/utils.ts (1)
233-250: Minor tidy-up: sort in place & tighten typings
newHeaders = newHeaders.sort(...)reallocates and loses type inference. Sorting in place is simpler:- newHeaders = newHeaders - .sort(([ka, _va], [kb, _vb]) => ka.localeCompare(kb)); + newHeaders.sort(([ka], [kb]) => ka.localeCompare(kb));tests/graphql/graphql_test.ts (1)
174-204: Flaky assertion risk – random collisions may trigger false failures
randIntreturnsMath.floor(Math.random() * 10000), and the test expects
assertNotEqualswhen idempotency is disabled. A 1 / 10 000 chance of collision exists, causing intermittent CI failures.Replace randomness with a deterministic counter or mock
Math.random():- randInt: () => Math.floor(Math.random() * 10000), + let counter = 0; + randInt: () => ++counter,or stub
Math.random()inside the test.docs/metatype.dev/docs/reference/typegate/requests/index.mdx (1)
24-28: Grammar nitpick“Keys expire happens after 24 hours.” → “Keys expire after 24 hours.”
- - Keys expire happens after 24 hours. + - Keys expire after 24 hours.🧰 Tools
🪛 LanguageTool
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...Post" } }' ``` :::info Notes - Keys expire happens after 24 hours. - A key must ma...(AI_HYDRA_LEO_MISSING_COMMA)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/metatype.dev/docs/reference/typegate/requests/index.mdx(1 hunks)src/typegate/src/services/graphql_service.ts(3 hunks)src/typegate/src/utils.ts(1 hunks)tests/graphql/graphql.py(1 hunks)tests/graphql/graphql_test.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/typegate/src/services/graphql_service.ts
🧰 Additional context used
🪛 LanguageTool
docs/metatype.dev/docs/reference/typegate/requests/index.mdx
[uncategorized] ~24-~24: Possible missing comma found.
Context: ...Post" } }' ``` :::info Notes - Keys expire happens after 24 hours. - A key must ma...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: pre-commit
🔇 Additional comments (1)
tests/graphql/graphql.py (1)
42-47: Verify upstream compatibility forrandIntThe proxy runtime points to
https://example.com/api/graphql; ensure that service actually exposes arandInt: Intfield. Otherwise this query will fail outside the mocked test environment.Confirm the remote schema or guard the feature behind configuration.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/typegate/src/typegate/register.ts (1)
177-188: TTL still ignored for replicated cache
addResponsesimply forwards toset, so expired entries persist in Redis forever.
Consider:
- Storing
expiryMillisas stream entry ID and purging stale ones onhistorySyncResponses.- Or leveraging Redis
EXPIREon a companion key.Without it, Redis size will grow unbounded.
src/typegate/src/utils.ts (2)
226-263: Still missing explicit typing fornewHeaders
let newHeaders = [];is inferred asany[]unlessnoImplicitAnyis off.- let newHeaders = []; + let newHeaders: Array<[string, string]> = [];This avoids possible lint errors and improves in-editor help.
265-291: Consider stripping volatile headers before serialising responses
Date,Set-Cookie,Server, etc. change per response and bloat the cache.
A quick filter intoSerializableResponsekeeps the store deterministic and smaller.src/typegate/src/services/graphql_service.ts (2)
177-183: Avoid unnecessary body consumption when cache misses
computeRequestSignaturereads the request body to hash it.
We call it unconditionally, then fetchregister.getResponse(key).
If the key is not in the register, we just wasted a body read (+ alloc) for nothing.Small optimisation:
- const userRequestHash = await computeRequestSignature(request, [ - IDEMPOTENCY_HEADER, - ]); - const now = Date.now(); - const memoized = await register.getResponse(key); + const now = Date.now(); + const memoized = await register.getResponse(key); + + // Only hash when we actually need to compare / store + const userRequestHash = memoized + ? await computeRequestSignature(request, [IDEMPOTENCY_HEADER]) + : undefined;This keeps hot-path latency lower and avoids double-reading large request bodies.
211-219: Hard-coded 24 h TTL could be made configurable
oneDay = 24 * 3600 * 1000is inlined.
Exposing it as aconstat the top of the file or reading from an env / config makes future tuning (e.g. shorter TTL for high-volume traffic) safer and avoids magic numbers.- const oneDay = 24 * 3600 * 1000; - const expiryMillis = now + oneDay; + const DEFAULT_IDEMPOTENCY_TTL = 24 * 60 * 60 * 1000; // 24 h + const expiryMillis = now + DEFAULT_IDEMPOTENCY_TTL;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/typegate/src/services/graphql_service.ts(3 hunks)src/typegate/src/typegate/memory_register.ts(2 hunks)src/typegate/src/typegate/mod.ts(2 hunks)src/typegate/src/typegate/register.ts(4 hunks)src/typegate/src/utils.ts(3 hunks)tests/utils/single_register.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/typegate/src/typegate/memory_register.ts (2)
src/typegate/src/engine/query_engine.ts (1)
QueryEngine(147-444)src/typegate/src/utils.ts (1)
CachedResponse(41-41)
tests/utils/single_register.ts (1)
src/typegate/src/utils.ts (1)
CachedResponse(41-41)
src/typegate/src/typegate/register.ts (2)
src/typegate/src/utils.ts (2)
CachedResponse(41-41)CachedResponseSchema(35-39)src/typegate/src/sync/replicated_map.ts (1)
RedisReplicatedMap(53-270)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: pre-commit
🔇 Additional comments (3)
src/typegate/src/typegate/mod.ts (1)
339-347: 👍 GraphQL handler refactor is cleanPassing the
registerallowshandleGraphQLto be storage-agnostic.src/typegate/src/utils.ts (1)
33-42: Schemas look good, but body type may exceed JSnumberrange
bodyisnumber[]; each entry represents a byte (0-255) so it is safe. Just flagging that large response bodies will allocate big arrays in JS/Redis.src/typegate/src/services/graphql_service.ts (1)
177-180: Consider excluding volatile headers (e.g.Authorization) from the signatureAt the moment only
Idempotency-Keyis skipped.
If the same logical request is retried with a refreshed bearer token, the hash will diverge and the 422 path will trigger.Typical practice is to hash the method + url + body and ignore headers whose values are expected to vary between retries (
Date,Authorization,User-Agent, etc.).
Worth re-evaluating based on your threat model.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/graphql/graphql_test.ts (2)
10-12: Unify assert imports for consistencyImport both
assertEqualsandassertNotEqualsfrom the same entry-point to avoid mixed styles.-import { assertEquals } from "@std/assert"; -import { assertNotEquals } from "@std/assert/not-equals"; +import { assertEquals, assertNotEquals } from "@std/assert";
13-30:syncConfigis duplicated – extract to a shared helperThis hard-coded Redis/S3 configuration also lives in other test helpers. Move it to a single module (e.g.
test-utils/config.ts) and import it here to keep the suite DRY and easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/typegate/src/typegate/memory_register.ts(3 hunks)src/typegate/src/typegate/register.ts(4 hunks)tests/graphql/graphql_test.ts(4 hunks)tests/utils/single_register.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/typegate/src/typegate/memory_register.ts
- tests/utils/single_register.ts
- src/typegate/src/typegate/register.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/graphql/graphql_test.ts (1)
tests/e2e/published/config.ts (2)
clearSyncData(60-62)setupSync(64-66)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
🔇 Additional comments (1)
tests/graphql/graphql_test.ts (1)
250-288: External dependencies may break local runsThe sync-mode test requires Redis and MinIO running with specific credentials. Add a guard (env flag or connection check) or document the requirement to avoid unexpected failures on environments without those services.
michael-0acf4
force-pushed
the
met-894-idempotent-request-by-key-in-custom-headers
branch
from
c7694d2 to
c39ff71
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/graphql/graphql_test.ts (2)
226-248: Order-coupled test remains – previous feedback not addressedThis block depends on cache state populated by the earlier “work” test inside the same
Meta.test. Reordering or running tests in isolation will break it. Initialise the cache inside the block or spin up a fresh engine.
73-75: Random source makes tests flaky
randIntrelies onMath.random(). The “idempotency” tests assume two uncached invocations return different numbers. With a 1/10 000 collision chance the suite will eventually fail on CI.Replace the resolver with a deterministic counter or inject a stubbed RNG:
- randInt: () => { - return Math.floor(Math.random() * 10000); - }, + // deterministic counter to eliminate flakiness in tests + randInt: (() => { + let _counter = 0; + return () => ++_counter; + })(),
🧹 Nitpick comments (2)
tests/graphql/graphql_test.ts (2)
10-12: Unused/debatable import path
assertNotEqualscan be imported directly from@std/assert(it re-exports all individual asserts). Keeping two different import paths for the same std lib fragment leads to drift elsewhere in the codebase.-import { assertNotEquals } from "@std/assert/not-equals"; +import { assertNotEquals } from "@std/assert";
13-30: Hard-coded sync credentials – move to shared test fixture
syncConfigis now duplicated in at least two test suites (tests/e2e/published/config.tsalready carries one). Centralising it avoids skew and cuts maintenance overhead.Consider exporting a single
getLocalSyncConfig()helper fromtest-utilsand reuse it here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/typegate/src/services/graphql_service.ts(3 hunks)src/typegate/src/typegate/memory_register.ts(3 hunks)src/typegate/src/typegate/register.ts(4 hunks)tests/graphql/graphql_test.ts(4 hunks)tests/utils/single_register.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/typegate/src/typegate/memory_register.ts
- tests/utils/single_register.ts
- src/typegate/src/services/graphql_service.ts
- src/typegate/src/typegate/register.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/graphql/graphql_test.ts (1)
tests/e2e/published/config.ts (2)
clearSyncData(60-62)setupSync(64-66)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
- GitHub Check: lint-compat (macos-13, x86_64-apple-darwin, false)
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
🔇 Additional comments (3)
tests/graphql/graphql_test.ts (3)
45-46: Schema extension LGTMAdding
randInttoQueryis consistent with the resolver below. No issues spotted.
194-224: Probability-based assertion still fragileLine 216’s
assertNotEquals(cached, value)can spuriously fail if two random ints collide (see previous comment). OncerandIntbecomes deterministic, this branch will always hold.After adopting a counter the assert should reverse — identical keys with empty/whitespace should still differ but will now do so deterministically. Adjust the expectation accordingly.
250-288: Sync-mode test looks solidSetup/teardown correctly clears the shared stores, and the header permutations cover the critical cases. ✅
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/graphql/graphql_test.ts (1)
228-249: Order-dependent cache expectation persists
Previous review already pointed out that"fail when reusing a known key"relies on cache state created by the"work"test. The inter-test dependency is still present and will break if the order flips.
Populate the cache inside this block or start a fresh engine instance to keep tests independent.
🧹 Nitpick comments (2)
tests/graphql/graphql_test.ts (2)
60-78: ResetnextCounterbetween tests to avoid hidden inter-suite coupling
nextCounteris module-level state. Its value survives across everyMeta.testblock, so assertions that depend on a specific integer sequence will silently influence one another when the test file is re-ordered or split.
Add abeforeEach(or reset inside each resolver) to keep suites hermetic.-let nextCounter = 0; +let nextCounter = 0; + +// Utility for tests – makes id generation deterministic per suite +function resetNextCounter() { + nextCounter = 0; +}Then call
resetNextCounter()in the setup of each suite.
This removes invisible dependencies and makes failures easier to triage.
265-267: Use deterministic data instead ofMath.random()for idempotency keysAlthough collisions are improbable, relying on
Math.random()introduces non-reproducibility. A simple monotonically increasing counter (or a hard-coded string unique to the test) removes the residual flake without changing semantics.-const key = Math.random().toString(); +const key = `sync-key-${Date.now()}`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/typegate/src/services/graphql_service.ts(3 hunks)tests/graphql/graphql.py(1 hunks)tests/graphql/graphql_test.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/graphql/graphql.py
- src/typegate/src/services/graphql_service.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/graphql/graphql_test.ts (1)
tests/e2e/published/config.ts (2)
clearSyncData(60-62)setupSync(64-66)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-full
- GitHub Check: build-docker (linux/arm64, ubuntu-22.04-arm)
- GitHub Check: lint-compat (macos-14, aarch64-apple-darwin, false)
- GitHub Check: build-docker (linux/amd64, custom-ubuntu-large)
michael-0acf4
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1031 +/- ##
==========================================
+ Coverage 80.57% 80.66% +0.08%
==========================================
Files 145 145
Lines 17341 17545 +204
Branches 1969 1980 +11
==========================================
+ Hits 13972 14152 +180
- Misses 3351 3375 +24
Partials 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
michael-0acf4
deleted the
met-894-idempotent-request-by-key-in-custom-headers
branch
Migration notes
None
Summary by CodeRabbit
New Features
nextIntthat returns an incrementing integer.Chores