Conversation
|
coverage: 100.0%. remained the same — limit-combinations into master |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR refactors the cartesian product generation in ChangesCartesian Product Limiting and Parameter Standardization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
RobinTail
force-pushed
the
limit-combinations
branch
from
94693b9 to
120fa0d
Compare
…security container processing.
…chema` (#3345) Performance regression since v24.0.0 - the `outputSchema` getter was calling the expensive `pullResponseExamples()` operation on first access (R.once protected), even though examples are only needed for OpenAPI documentation generation (getResponses). This fixes the issue where simply accessing the output schema for runtime validation was unnecessarily triggering example generation. first introduced by #2666 merged by #2546 found this during my work on #3344 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved request performance by deferring output schema example processing; examples are now generated only when needed for documentation/OpenAPI workflows, not during initial request handling. * **Documentation** * Added changelog entry v27.2.4 documenting the change in example processing behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@express-zod-api/src/documentation-helpers.ts`:
- Around line 129-133: depictIntersection reads maxCombinations from the
depiction context but depictResponse and depictRequest build ctx without that
field, causing intersections to run unbounded; update the context construction
in depictResponse and depictRequest to include maxCombinations (propagate the
value from the function arguments/config/options into the ctx passed to
depictIntersection and flattenIO) so that flattenIO receives the intended
maxCombinations limit.
In `@express-zod-api/src/documentation.ts`:
- Around line 89-94: The JSDoc for the public constructor option maxCombinations
is missing an `@example`; update the comment block for the maxCombinations
property in documentation.ts to include an `@example` directive showing typical
usage (e.g., setting maxCombinations: 20 or Infinity) while keeping the existing
`@desc` and `@default` lines so the property has `@desc`, `@default`, and `@example` as
required by the public API documentation guidelines.
In `@express-zod-api/src/endpoint.ts`:
- Around line 94-98: Remove the R.once memoization wrapper from the private
methods that generate examples so they can be re-run with different limits:
specifically un-wrap the definitions of `#ensureOutputExamples` and the analogous
`#ensureInputExamples` (the ones currently defined as R.once((limit?: number) => {
... })). Leave the existing guard checks (e.g., the globalRegistry lookup and
isSchema check inside `#ensureOutputExamples`) intact so redundant work is still
avoided, but allow the function body to execute on each call with the provided
limit parameter instead of being permanently memoized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99c13e7d-1c60-4cf9-8e92-1770c8851336
📒 Files selected for processing (14)
express-zod-api/src/common-helpers.tsexpress-zod-api/src/diagnostics.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/documentation.tsexpress-zod-api/src/endpoint.tsexpress-zod-api/src/integration.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/logical-container.tsexpress-zod-api/src/result-helpers.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/endpoint.spec.tsexpress-zod-api/tests/json-schema-helpers.spec.tsexpress-zod-api/tests/logical-container.spec.tsexpress-zod-api/tests/result-helpers.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
express-zod-api/src/documentation.ts (1)
170-176: 💤 Low valueGlobal state mutation may cause interference between concurrent documentation generations.
Setting
combinations.limitas a module-level side effect means the limit persists across calls and could affect other code paths that usecombinations(). If multipleDocumentationinstances are created (e.g., in tests or parallel generation), the last-set limit wins.Consider whether this is acceptable for the intended use case. If the documentation generation is always synchronous and single-instance, this is fine. Otherwise, a scoped approach (passing the limit through context) would be safer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@express-zod-api/src/documentation.ts` around lines 170 - 176, The constructor of the Documentation class mutates the module-level combinations by setting combinations.limit, which creates global state leakage; instead remove that side-effect and scope the limit to this instance by creating a local combinations object or passing the limit through a parameter/context when generating docs (e.g., instantiate a localCombinations from combinations() or call combinations({limit: maxCombinations}) if supported), update any uses in Documentation methods to reference the instance-local combination object rather than the module-level combinations symbol, and ensure no other code relies on the module-level combinations.limit being modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@express-zod-api/src/diagnostics.ts`:
- Around line 49-52: The diagnostics code mutates the shared combinations.limit
(setting it to 0) and fails to restore it, causing later consumers to see the
wrong limit; update the sections that set combinations.limit (the block
iterating responseVariants + endpoint.getResponses(...) and the similar block in
`#checkPathParams`) to save the original value (e.g., const prevLimit =
combinations.limit) before changing it and restore it in a finally or after the
loop (combinations.limit = prevLimit) so the global state is not left modified
after diagnostics complete.
In `@express-zod-api/src/integration.ts`:
- Line 95: The code sets combinations.limit = 0 and leaves module state mutated;
fix by saving the original value (e.g., const prev = combinations.limit) before
setting it to 0, perform the generation, and restore combinations.limit = prev
in a finally block so the original limit is always reinstated; apply this change
in the same constructor/generation block where combinations.limit is modified to
avoid shared-state side effects.
In `@express-zod-api/tests/common-helpers.spec.ts`:
- Around line 294-298: The test mutates the global combinations.limit and only
resets it if the assertions pass; change the test to always restore
combinations.limit (e.g., capture const prev = combinations.limit then run the
assertions inside a try/finally and in finally set combinations.limit = prev) so
the global state is restored even on failure; refer to the combinations function
and its combinations.limit property when making the change.
---
Nitpick comments:
In `@express-zod-api/src/documentation.ts`:
- Around line 170-176: The constructor of the Documentation class mutates the
module-level combinations by setting combinations.limit, which creates global
state leakage; instead remove that side-effect and scope the limit to this
instance by creating a local combinations object or passing the limit through a
parameter/context when generating docs (e.g., instantiate a localCombinations
from combinations() or call combinations({limit: maxCombinations}) if
supported), update any uses in Documentation methods to reference the
instance-local combination object rather than the module-level combinations
symbol, and ensure no other code relies on the module-level combinations.limit
being modified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3231cda2-0a60-4f50-8cb8-d611f6a1015c
📒 Files selected for processing (11)
express-zod-api/src/common-helpers.tsexpress-zod-api/src/diagnostics.tsexpress-zod-api/src/documentation-helpers.tsexpress-zod-api/src/documentation.tsexpress-zod-api/src/endpoint.tsexpress-zod-api/src/integration.tsexpress-zod-api/src/json-schema-helpers.tsexpress-zod-api/src/logical-container.tsexpress-zod-api/src/result-helpers.tsexpress-zod-api/tests/common-helpers.spec.tsexpress-zod-api/tests/json-schema-helpers.spec.ts
There was a problem hiding this comment.
Caution
The R.once wrapper on #ensureOutputExamples is being re-introduced after being removed to fix this exact issue in a previous review. In a single process that both runs the server in development and instantiates Documentation over the same endpoint instances, Diagnostics.#checkSchema sets combinations.limit = 0 and triggers getResponses("positive") first — pullResponseExamples returns [], the if (!examples.length) return guard exits, and R.once permanently memoizes the call. When Documentation later runs getResponses("positive") with combinations.limit = maxCombinations, the memoized method is a no-op and output examples are never pulled up into the registry.
Reviewed changes:
- Restored the
R.once()wrapper aroundEndpoint#ensureOutputExamples(thelimitparameter no longer exists on the method since the whole per-calllimitplumbing was replaced by the globalcombinations.limitin earlier commits).
Note: 1 inline comment(s) dropped because they did not anchor to lines inside the PR diff:
express-zod-api/src/endpoint.ts:93-102(RIGHT) — file not in PR diff
Claude Opus ｜ 𝕏
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Reverted the
R.oncewrapper onEndpoint#ensureOutputExamples, restoring it to a plain method so each call re-evaluates against the currentcombinations.limit; theglobalRegistrycheck inside the method still prevents redundant work
Prior review feedback:
-
R.onceon#ensureOutputExamplessilently drops thelimitparameter undercombinations.limit = 0fromDiagnostics— addressed by reverting commit8a98629a
Claude Opus ｜ 𝕏
|
Verified the fix in commit
|
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Moved the
combinations.limit = 0mutation out ofDiagnosticsand intocollectSiblingsinrouting.ts, which now savesprevLimit, scopes the zero-limit to the entirewalkRoutingpass (covering both#checkSchemaand#checkPathParams), and restores the previous value afterward diagnostics.tsno longer touchescombinations.limitdirectly, so the module-state mutation is now confined to a single scoped block at the routing boundary
Prior review feedback:
-
Diagnosticsmutatingcombinations.limitwithout restoring it — addressed by save/restore incollectSiblings
Claude Opus ｜ 𝕏
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Acknowledged the fix in 258511a (save/restore of
|
|
Verified the fix in a7cb6bd —
|
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Added save/restore of
combinations.limitaround theDocumentationconstructor body, mirroring the pattern already in place incollectSiblings(routing.ts) and the earlierIntegrationcommit, so amaxCombinationsvalue set during documentation generation no longer leaks into subsequentcombinations()call sites in the same process.
Note: the prior review on Integration (early return at integration.ts:167 bypassing the restore at line 183 for variant === "types") remains unaddressed by this commit.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
No new issues. Reviewed the following changes:
- Moved
combinations.limit = prevLimitinIntegrationso it runs before theif (variant === "types") return;early exit, ensuring the restore fires on both the"client"and"types"paths. All schema-processing that relies oncombinations.limit = 0has completed by the time of the restore (it sits afterwalkRoutingand the#program.push(...)of type declarations), and the remainingmake*helpers below the early return do not callcombinations().
Prior review feedback:
-
combinations.limit = 0leaks on thevariant === "types"path inIntegration— addressed by moving the restore above the earlyreturn
Claude Opus ｜ 𝕏
|
Verified the fix in bfaa816 —
|
|
Confirmed the
|
|
@pullfrog evaluate the design of |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
RobinTail
force-pushed
the
limit-combinations
branch
from
bfaa816 to
9251a67
Compare
|
RobinTail confirmed the original review feedback was addressed by reverting to an explicit
|
|
No code changes needed — RobinTail's reply "reverted" on the
|
|
Acknowledged RobinTail's "reverted" reply on the outdated
|
|
Reviewer confirmed the concern was addressed by reverting the global-mutation approach in favour of passing
|
|
Replied to the
|
|
Acknowledged RobinTail's
|
|
Acknowledged the reviewer's "reverted" reply on the
|
There was a problem hiding this comment.
Caution
The maxCombinations: 0 sentinel used internally by Diagnostics silently corrupts globalRegistry for output schemas with an odd number of example-bearing properties, and the corruption persists into the subsequent Documentation pass — so users running in non-production mode get degenerate response examples regardless of the maxCombinations they configure. The same limit=0 semantic also wipes security alternatives when the endpoint has two or more or containers, and produces parity-dependent request examples in flattenIO. All three stem from the empty-array short-circuit in combinations ignoring limit.
The refactor and the feature are otherwise sound. Performance is neutral-or-better; integration impact is contained to @internal surfaces. The blocker is the interaction between the internal { maxCombinations: 0 } sentinel and the new "limit ignored on concat" contract — that combination was not the intent of either side.
Reproducer for the registry-poisoning path (simulating pullResponseExamples reducing over 1/2/3 property schemas at limit=0):
1 prop: [ { a: 1 } ] ← written to registry
2 props: [] ← early-return in #ensureOutputExamples, OK
3 props: [ { c: 3 } ] ← written to registry, drops a and b entirely
4 props: [] ← OK
Suggested directions (pick whichever fits the design):
- Split "compute examples" from "cache to registry" so
Diagnostics/Integrationnever mutate the registry. Or let#ensureOutputExamplesskip the write whenlimit === 0. - Exempt
processContainers(endpoint.security, ...)frommaxCombinations— security alternatives are semantic requirements, not examples. - Either make
limit=0oncombinationsreturn[]unconditionally (including on concat), or change thepullRequestExamples/pullResponseExamples/flattenIOreducers solimit=0means "skip the product step entirely" rather than relying oncombinationsto honor it.
Additional note: there is no end-to-end test for Documentation({ ..., maxCombinations: N }) — a snapshot over a schema with multi-example request and response would have caught the registry-poisoning interaction and the security-wipe.
Claude Opus ｜ 𝕏
| } | ||
| for (const variant of responseVariants) { | ||
| for (const { mimeTypes, schema } of endpoint.getResponses(variant)) { | ||
| const responses = endpoint.getResponses(variant, { maxCombinations: 0 }); // no examples |
There was a problem hiding this comment.
This { maxCombinations: 0 } is used as a sentinel for "don't compute examples", but getResponses("positive", ...) still calls #ensureOutputExamples(0), which still calls pullResponseExamples(schema, 0), which still writes to globalRegistry whenever the reduce ends on the empty-array branch. For output schemas with an odd number of example-bearing properties the registry ends up with a single-property example (the last one), and for even counts it stays empty. Because Diagnostics runs unconditionally in non-production during initRouting (routing.ts:70), every subsequent Documentation generation then hits the globalRegistry.get(...)?.examples?.length short-circuit at endpoint.ts:95 and silently reuses the degenerate examples — regardless of the user's configured maxCombinations. Either gate the registry write by limit !== 0, or give Diagnostics a path that doesn't mutate shared state.
There was a problem hiding this comment.
@pullfrog wait a second, there is if (!examples.length) return; statement on line 98 that prevents writing into registry an empty array of examples.
There was a problem hiding this comment.
Pushed back on the reviewer's question with evidence that my original concern stands. Verified locally: pullResponseExamples(z.object({ a, b, c }), 0) returns [{ c: 3 }] (non-empty) because combinations' empty-side shortcut bypasses the limit check, so the reduce "resets" to each property's examples whenever acc is empty. The if (!examples.length) return; guard only catches the even-count case. No code changes made; left the thread open for the author.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
The if (!examples.length) return; guard does catch the even-count case, but it misses the odd-count case. With limit=0, pullResponseExamples reduces with combinations(acc, propExamples, R.mergeRight, 0) — and combinations shortcircuits via if (!left.length || !right.length) return left.concat(right); before the limit is ever consulted. So when acc is empty and the next property has examples, the reduce "resets" to that property's examples (ignoring the limit), then the next non-empty property flips it back to [], and so on.
Confirmed with a direct call in this branch: pullResponseExamples(z.object({ a, b, c }), 0) (where each prop has one example) returns [{ c: 3 }] — non-empty, bypassing the guard and writing the degenerate single-property example into globalRegistry. An even count ({ a, b }) correctly ends at [] and is caught by the guard.
Two possible fixes: early-return from pullResponseExamples when limit === 0, or tighten combinations so the empty-side shortcut also respects a 0 limit.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
I've got your point on odd/even number of properties and how it may affect the result.
| const securityRefs = depictSecurityRefs( | ||
| depictSecurity(security, inputSources), | ||
| depictSecurity( | ||
| processContainers(endpoint.security, maxCombinations), |
There was a problem hiding this comment.
Threading maxCombinations into processContainers for the security tree conflates "example combinations" with "required-auth alternatives". For { or: [A,B] }, { or: [C,D] } security at maxCombinations: 0, the reduce inside processContainers produces [] (the second step hits the non-empty-×-non-empty branch with limit=0), and depictSecurity then emits security: [] on the OpenAPI operation — i.e. no auth documented for an endpoint that actually requires one of several. Security alternatives should not be subject to the example-combinations limit; call processContainers(endpoint.security) without the limit here.
| /** @desc Maximum number of combinations (only applies to Cartesian product of non-empty arrays) */ | ||
| limit = Infinity, | ||
| ): T[] => { | ||
| if (!left.length || !right.length) return left.concat(right); |
There was a problem hiding this comment.
The empty-array short-circuit intentionally ignores limit, which is fine as a local contract for combinations but leaks surprising behavior into every caller that reduces over a list (pullRequestExamples, pullResponseExamples, mergeExamples via flattenIO, processContainers). At limit=0, a reducer alternates between "ignore the limit and copy right wholesale" on empty-left iterations and "return []" on non-empty iterations, producing parity-dependent output — e.g. a 3-property request schema keeps only the last property's examples while a 2-property one returns none. The CHANGELOG description "Set to 0 to disable product but keep concatenations" doesn't match this behavior for multi-property schemas. Either make limit=0 short-circuit to [] here, or restructure the reducers so limit=0 skips the product step entirely rather than piggy-backing on combinations.
| variant: ResponseVariant, | ||
| { maxCombinations }: { maxCombinations?: number }, | ||
| ) { | ||
| if (variant === "positive") this.#ensureOutputExamples(maxCombinations); |
There was a problem hiding this comment.
With R.once gone, idempotency now relies on globalRegistry.get(...)?.examples?.length at line 95. That makes this method first-caller-wins: whichever caller (Diagnostics, Integration, Documentation) hits a given outputSchema first pins the examples for every subsequent caller on the same process. In the current call graph, Diagnostics (maxCombinations: 0) runs first in non-production — see the related comment on diagnostics.ts:50. Worth pinning this contract with a test that calls getResponses twice with different maxCombinations and asserts the intended behavior, and adding at least one test covering getResponses("positive", { maxCombinations: <positive> }) which no existing spec exercises.
1 participant
Summary by CodeRabbit
Release Notes
New Features
maxCombinationsconfiguration option to the Documentation generator, enabling control over example combination generation. Set to0to suppress example generation; defaults to unlimited.Improvements