fix: don't expose public types #3494
Conversation
📝 WalkthroughWalkthroughAdds a new internal /types export endpoint and ErrorExtension model to specs and clients; standardizes problem+json errors with an extensions.validationErrors array; introduces a Go helper to parse extensions; updates server/common HTTP error mapping and product catalog validation errors to carry 400 status; adjusts related handlers and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
GAlexIHU
force-pushed
the
fix/subs/multi-subs/error-extension-private-fields
branch
from
c4b5731 to
ffe71cf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/framework/commonhttp/errors.go (1)
175-183: Avoid mutating original issue attributes; clone before deletingissue.Attributes() likely returns the backing map. Deleting in-place can leak side effects. Clone first, then delete.
- // We don't want to expose private attributes to the client - attrs := issue.Attributes() - delete(attrs, httpStatusCodeErrorAttribute) - delete(attrs, issue.Code()) - issue = issue.SetAttributes(attrs) + // We don't want to expose private attributes to the client; clone before mutating + orig := issue.Attributes() + attrs := models.Attributes{} + for k, v := range orig { + attrs[k] = v + } + delete(attrs, httpStatusCodeErrorAttribute) + delete(attrs, issue.Code()) + issue = issue.SetAttributes(attrs)api/spec/src/productcatalog/errors.tsp (1)
10-17: Use CommonSubscriptionErrors for all subscription-related routes
- api/spec/src/productcatalog/routes.tsp:583 (restore subscription)
- api/spec/src/productcatalog/routes.tsp:664 (list subscription addons)
- api/spec/src/productcatalog/routes.tsp:676 (get subscription addon)
- api/spec/src/productcatalog/routes.tsp:691 (create/update subscription addon)
🧹 Nitpick comments (6)
pkg/models/testutils.go (1)
24-25: Consider clarifying the comment phrasing.The comment states "Do not strip attributes" immediately before code that nils the field, which could be misinterpreted. Consider rephrasing to make it clearer that attributes are compared in full prior to nilding the field reference.
Suggested alternative phrasing:
-// Do not strip attributes; compare full set, including status code +// Attributes (including status code) are compared via the assertions above. +// Only nil the field reference after comparison to allow struct equality check.pkg/framework/commonhttp/errors.go (3)
141-143: Minor: log message grammar“issue does have …” → “issue has …”.
- slog.Default().DebugContext(ctx, "issue does have HTTP status code attribute but it's not an integer", "issue", issue) + slog.Default().DebugContext(ctx, "issue has HTTP status code attribute but it's not an integer", "issue", issue)
168-172: Comment contradicts behavior; update to reflect “validationErrors”Code sets key to “validationErrors”, but the comment says “validationIssues”. Update the comment.
- // For backwards compatibility across server responses we expose - // validation issues under "validationIssues" for 400 responses. + // For backwards compatibility across server responses we expose + // validation issues under "validationErrors" for 400 responses.
163-186: Future-proof closure over loop variablesSafe today (singular behavior ensures one iteration), but if behavior changes, closures will capture the last values. Capture per-iteration values.
- for code, issues := range issuesByCodeMap { + for code, issues := range issuesByCodeMap { + code, issues := code, issues // capture loop variables responseStatusCode = code extendProblemFuncs = append(extendProblemFuncs, func() map[string]interface{} { codeStr := strconv.Itoa(code) // ... }) }api/openapi.cloud.yaml (1)
11878-11895: Ensure/typesendpoint is hidden from public artifacts and SDKs
Many generators only respectx-internalat the operation level. To prevent accidental exposure:
- Add
x-internal: trueat the path level (in addition to thegetoperation).- Assign a dedicated internal tag (with
x-internal: true) so doc tools will hide it.- Optional: reference export-only schemas via a real component (using
oneOf/allOfanchors) instead of a fake path to avoid tree-shaking.api/openapi.yaml (1)
17312-17333: Relaxfieldrequirement on ErrorExtension.Line 17312 makes
fieldmandatory even though the schema is described as a generic HTTPProblem extension. Several status-code-specific extensions (conflict, permission, throttling, etc.) do not have a pinpointed field, so this constraint forces callers to fabricate dummy values to stay spec-compliant. Please dropfieldfrom the required set (or make it optional) so we can reuse this schema for non-field-scoped errors.required: - - field - code - message
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/client/go/error.go(2 hunks)api/client/javascript/src/client/schemas.ts(12 hunks)api/openapi.cloud.yaml(10 hunks)api/openapi.yaml(9 hunks)api/spec/Makefile(1 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/errors.tsp(2 hunks)api/spec/src/main.tsp(1 hunks)api/spec/src/productcatalog/errors.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(7 hunks)e2e/productcatalog_test.go(3 hunks)openmeter/productcatalog/errors.go(47 hunks)openmeter/productcatalog/planaddon_test.go(6 hunks)openmeter/productcatalog/subscription/http/errors.go(2 hunks)openmeter/subscription/errors.go(10 hunks)openmeter/subscription/errors_test.go(10 hunks)pkg/framework/commonhttp/errors.go(2 hunks)pkg/framework/commonhttp/errors_test.go(1 hunks)pkg/models/testutils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/productcatalog/errors.go
🧬 Code graph analysis (10)
api/client/javascript/src/client/schemas.ts (3)
pkg/models/validationissue.go (1)
ErrorExtension(11-11)api/api.gen.go (1)
ErrorExtension(3422-3432)api/client/go/client.gen.go (1)
ErrorExtension(3136-3146)
openmeter/productcatalog/planaddon_test.go (3)
pkg/models/validationissue.go (3)
NewValidationIssue(273-283)WithField(216-222)WithSeverity(234-238)pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)pkg/models/fielddescriptor.go (1)
NewFieldSelector(213-218)
api/client/go/error.go (3)
api/client/javascript/src/client/schemas.ts (1)
ErrorExtension(11652-11652)api/api.gen.go (1)
ErrorExtension(3422-3432)api/client/go/client.gen.go (1)
ErrorExtension(3136-3146)
pkg/framework/commonhttp/errors.go (2)
pkg/models/validationissue.go (1)
ValidationIssue(18-25)pkg/models/attributes.go (1)
Attributes(8-8)
pkg/framework/commonhttp/errors_test.go (3)
pkg/models/errors.go (1)
ErrorCode(294-294)pkg/models/validationissue.go (1)
NewValidationIssue(273-283)pkg/framework/commonhttp/errors.go (2)
WithHTTPStatusCodeAttribute(83-85)HandleIssueIfHTTPStatusKnown(114-196)
openmeter/subscription/errors_test.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
openmeter/subscription/errors.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
e2e/productcatalog_test.go (1)
api/client/go/error.go (1)
ParseExtensionsFromInterface(20-30)
openmeter/productcatalog/subscription/http/errors.go (3)
pkg/models/validationissue.go (2)
AsValidationIssues(331-342)ValidationIssues(295-295)pkg/framework/commonhttp/errors.go (1)
HandleIssueIfHTTPStatusKnown(114-196)openmeter/subscription/fielddescriptormapper.go (1)
MapSubscriptionSpecValidationIssueField(19-53)
openmeter/productcatalog/errors.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Code Generators
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
api/spec/src/errors.tsp (2)
55-65: Extensions property addition is clearAdding Error.extensions for typed payloads aligns with RFC7807 extensions practice and your usage.
140-167: Generic ErrorExtension model looks goodFields/code/message + passthrough meet needs for various error flavors.
Double-check client parsers handle both:
- extensions.validationErrors: ValidationError[]
- extensions[""]: ErrorExtension[]
If needed, I can draft a brief conformance test.
openmeter/subscription/errors.go (1)
23-24: All subscription validation issues include HTTP status attributes
No package-level NewValidationIssue calls in openmeter/subscription/errors.go lack commonhttp.WithHTTPStatusCodeAttribute.api/openapi.cloud.yaml (6)
9759-9763: Replicated 400 mapping: consistency checkSame note as above: confirm BadRequestProblemResponse carries the typed, sanitized extensions.
10309-10313: Change/response: BadRequest linkageApproved. Double-check that SubscriptionChange errors also emit typed extensions (no extra fields).
10394-10398: Repeat 400 mappingSame as above; keeping for completeness.
10574-10578: 400 mapping approvedNo issues spotted; see global verification notes.
10220-10224: Uniform 400 problem response mapping verified GenericValidationError is handled as HTTP 400 in GenericErrorEncoder.
9836-9840: 400 responses useBadRequestProblemResponse; no endpoints reference legacyValidationErrorProblemResponseorValidationErrorschemas.api/spec/src/productcatalog/routes.tsp (1)
489-489: Align SubscriptionAddons error unions with CommonSubscriptionErrors
createSubscriptionAddon,listSubscriptionAddons, andupdateSubscriptionAddonstill referenceOpenMeter.CommonErrors; switch them to useCommonSubscriptionErrorsfor consistent error handling.api/client/javascript/src/client/schemas.ts (6)
2438-2454: LGTM! Phantom endpoint for type export is well-documented.The comment clearly explains that this
/typesendpoint is a codegen artifact and doesn't represent a real API resource. This approach is a pragmatic solution for exporting tree-shaken types through the OpenAPI spec.
5628-5647: LGTM! ErrorExtension schema is well-structured.The schema correctly defines the error extension structure with appropriate readonly modifiers and additional properties support. The structure aligns with the Go models shown in the codebase.
11652-11652: LGTM!
26996-27017: LGTM! Exports operation correctly references ErrorExtension schema.The
Exports_exportsoperation properly defines the response structure for the phantom/typesendpoint, enabling codegen tools and SDKs to discover and export theErrorExtensiontype.
24269-24276: Confirm 400 error response type change with clientsThe
BadRequestProblemResponseschema (now used for all 400 responses) includes an optionalextensions?: { [key: string]: unknown }. Verify that existing clients handle this response type change gracefully or document it as a breaking change.
10974-10988: Genericextensionstyping is correct –Record<string, unknown>accommodates any ErrorExtension payload (e.g.validationErrorsarrays); field-specific parsing is handled via existing helpers.
| description: The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing). | ||
| content: | ||
| application/problem+json: | ||
| schema: | ||
| $ref: '#/components/schemas/ValidationErrorProblemResponse' | ||
| $ref: '#/components/schemas/BadRequestProblemResponse' |
There was a problem hiding this comment.
🧩 Analysis chain
400 → BadRequestProblemResponse: verify typed, privacy-safe extensions
Looks good. Please confirm BadRequestProblemResponse defines extensions with the new typed flavor(s) so private fields aren’t exposed.
Run:
🏁 Script executed:
#!/bin/bash
# Ensure BadRequestProblemResponse exists and includes `extensions`
rg -nC2 -g 'api/**' 'components:\s*\n(?:.*\n)*?schemas:\s*\n(?:.*\n)*?BadRequestProblemResponse' || true
rg -nC3 --type=yaml 'BadRequestProblemResponse' | rg -nC2 'extensions:'Length of output: 191
🏁 Script executed:
#!/bin/bash
# Locate the BadRequestProblemResponse schema definition
rg -n '^ +BadRequestProblemResponse:' -A10 -g 'api/openapi.cloud.yaml' || true
# Check for any extensions field in the file
rg -n 'extensions:' -g 'api/openapi.cloud.yaml' || trueLength of output: 836
🏁 Script executed:
#!/bin/bash
# Extract UnexpectedProblemResponse definition
rg -n '^ +UnexpectedProblemResponse:' -A15 -g 'api/openapi.cloud.yaml' || true
# Show context around the extensions field
sed -n '24150,24200p' api/openapi.cloud.yaml || trueLength of output: 2490
Restrict Problem.extensions to a typed schema
BadRequestProblemResponse currently inherits extensions as a free-form object from UnexpectedProblemResponse (api/openapi.cloud.yaml lines 24131–24141), risking exposure of private data. Define a dedicated ProblemExtensions schema with allow-listed properties (e.g., validationErrors) and reference it instead of using type: object + additionalProperties: {}.
| ErrorExtension: | ||
| type: object | ||
| required: | ||
| - field | ||
| - code | ||
| - message | ||
| properties: | ||
| field: | ||
| type: string | ||
| description: The path to the field. | ||
| example: addons/pro/ratecards/token/featureKey | ||
| readOnly: true | ||
| code: | ||
| type: string | ||
| description: The machine readable description of the error. | ||
| example: invalid_feature_key | ||
| readOnly: true | ||
| message: | ||
| type: string | ||
| description: The human readable description of the error. | ||
| example: not found feature by key | ||
| readOnly: true | ||
| additionalProperties: {} | ||
| description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode] | ||
| Event: |
There was a problem hiding this comment.
ErrorExtension is open-ended (additionalProperties: {}): potential private data leak
Allowing arbitrary properties undermines the goal of not exposing private error details.
Recommend closing the shape or scoping extras under a dedicated, typed map:
- ErrorExtension:
- type: object
- required:
- - field
- - code
- - message
- properties:
+ ErrorExtension:
+ type: object
+ required:
+ - field
+ - code
+ - message
+ properties:
field:
type: string
description: The path to the field.
example: addons/pro/ratecards/token/featureKey
readOnly: true
code:
type: string
description: The machine readable description of the error.
example: invalid_feature_key
readOnly: true
message:
type: string
description: The human readable description of the error.
example: not found feature by key
readOnly: true
- additionalProperties: {}
+ additionalProperties: false
+ # If arbitrary attributes are needed, gate them explicitly:
+ # properties:
+ # attributes:
+ # type: object
+ # additionalProperties:
+ # type: string
+ # description: Non-sensitive attributes relevant to the error.
description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode]Also consider whether field can reveal internal paths; if so, document a public path format and normalize server-side.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ErrorExtension: | |
| type: object | |
| required: | |
| - field | |
| - code | |
| - message | |
| properties: | |
| field: | |
| type: string | |
| description: The path to the field. | |
| example: addons/pro/ratecards/token/featureKey | |
| readOnly: true | |
| code: | |
| type: string | |
| description: The machine readable description of the error. | |
| example: invalid_feature_key | |
| readOnly: true | |
| message: | |
| type: string | |
| description: The human readable description of the error. | |
| example: not found feature by key | |
| readOnly: true | |
| additionalProperties: {} | |
| description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode] | |
| Event: | |
| ErrorExtension: | |
| type: object | |
| required: | |
| - field | |
| - code | |
| - message | |
| properties: | |
| field: | |
| type: string | |
| description: The path to the field. | |
| example: addons/pro/ratecards/token/featureKey | |
| readOnly: true | |
| code: | |
| type: string | |
| description: The machine readable description of the error. | |
| example: invalid_feature_key | |
| readOnly: true | |
| message: | |
| type: string | |
| description: The human readable description of the error. | |
| example: not found feature by key | |
| readOnly: true | |
| additionalProperties: false | |
| # If arbitrary attributes are needed, gate them explicitly: | |
| # properties: | |
| # attributes: | |
| # type: object | |
| # additionalProperties: | |
| # type: string | |
| # description: Non-sensitive attributes relevant to the error. | |
| description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode] | |
| Event: |
🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 16955-16979, the ErrorExtension schema
currently permits arbitrary properties via additionalProperties: {}, which risks
leaking private/internal error details; change additionalProperties to false (or
remove it) to close the shape, or alternatively replace it with a typed map
property (e.g., "extras": { type: object, additionalProperties: { type: string }
}) to explicitly scope allowed extra metadata; also audit the "field" property
and, if it may expose internal paths, document an external/public field-path
format in the schema description and ensure the server normalizes/rewrites
internal paths to that public format before returning errors.
| extensions: | ||
| type: object | ||
| additionalProperties: {} | ||
| description: Additional properties specific to the problem type may be present. | ||
| example: | ||
| validationErrors: | ||
| - code: validation_error | ||
| message: Validation error | ||
| otherAttribute: otherValue |
There was a problem hiding this comment.
Problem.extensions is untyped and open (additionalProperties: {}): reintroduces leakage risk
Defining extensions as a free-form object can carry any server-internal fields. Align with the PR’s “typed flavors” by allowlisting keys and types.
Replace the generic object with a typed schema and disallow extra keys:
- extensions:
- type: object
- additionalProperties: {}
- description: Additional properties specific to the problem type may be present.
- example:
- validationErrors:
- - code: validation_error
- message: Validation error
- otherAttribute: otherValue
+ extensions:
+ $ref: '#/components/schemas/ProblemExtensions'Add this schema under components/schemas (outside this hunk):
components:
schemas:
ProblemExtensions:
type: object
properties:
validationErrors:
type: array
items:
$ref: '#/components/schemas/ErrorExtension'
# Add more well-known, non-sensitive keys here, e.g.:
# requestId:
# type: string
additionalProperties: false
description: Allowlisted problem extensions safe for clients.Note: The example uses validationErrors; ensure naming matches server encoders and tests. If you intend HTTPProblem.Extensions.[StatusCode], reflect that structure here or provide a union/oneOf.
🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 24158-24166, the Problem.extensions
schema is defined as an open object (additionalProperties: {}), which allows
arbitrary server-internal fields; replace it with a typed, allowlisted schema by
creating a new components/schemas/ProblemExtensions definition (type: object,
properties including validationErrors as an array of ErrorExtension,
additionalProperties: false, and a clear description) and then change the
Problem.extensions reference to use this new ProblemExtensions schema; ensure
the example is updated to match the typed fields and that the property names
match server encoders/tests.
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/spec/src/productcatalog/routes.tsp (1)
481-481: ReplaceOpenMeter.CommonErrorswithCommonSubscriptionErrorsingetExpanded.
Update the return type ofgetExpandedtoSubscriptionExpanded | OpenMeter.NotFoundError | CommonSubscriptionErrorsto match all other subscription endpoints.
♻️ Duplicate comments (13)
api/client/javascript/src/client/schemas.ts (7)
24458-24465: Same as earlier: runtime must handleapplication/problem+json
24556-24563: Same as earlier: runtime must handleapplication/problem+json
25061-25068: Same as earlier: runtime must handleapplication/problem+json
25168-25175: Same as earlier: runtime must handleapplication/problem+json
25294-25301: Same as earlier: runtime must handleapplication/problem+json
25491-25498: Same as earlier: runtime must handleapplication/problem+json
26996-27017: Duplicate of/typesconcern: keep operation internal (no public SDK function)Ensure
Exports_exportsis filtered from generated client code (see earlier verification script).api/openapi.yaml (6)
9760-9764: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
9837-9841: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
10221-10225: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
10310-10314: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
10395-10399: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
10575-10579: Same as Lines 9605-9609: reuse a components/responses 400 and verify extensions are included.
🧹 Nitpick comments (7)
openmeter/productcatalog/subscription/http/errors.go (1)
22-24: Update misleading comment and consider explicit mapping‐failure handling.The inline comment
// Server dies if mapping failsis incorrect: returningfalsesimply falls back to the generic error handlers rather than terminating the server. Remove or update this comment. If a mapping error should yield a 500 response, add explicit logging and aw.WriteHeader(http.StatusInternalServerError)before returning.api/client/go/error.go (1)
20-30: Make parsing helper safer and more ergonomic (doc, short-circuit, error wrapping, useany).
- Add doc comment.
- Accept
any.- Short-circuit when already
[]ErrorExtension.- Wrap errors with context.
Apply:
-import ( - "encoding/json" -) +import ( + "encoding/json" + "fmt" +) @@ -func ParseExtensionsFromInterface(in interface{}) ([]ErrorExtension, error) { - // Let's ser/de - str, err := json.Marshal(in) - if err != nil { - return nil, err - } - - var out []ErrorExtension - err = json.Unmarshal(str, &out) - return out, err -} +// ParseExtensionsFromInterface converts an arbitrary value (e.g. []map[string]any, +// json.RawMessage, etc.) into a slice of ErrorExtension. +// Returns a nil slice when input is nil. +func ParseExtensionsFromInterface(in any) ([]ErrorExtension, error) { + if exts, ok := in.([]ErrorExtension); ok { + return exts, nil + } + b, err := json.Marshal(in) + if err != nil { + return nil, fmt.Errorf("marshal extensions: %w", err) + } + var out []ErrorExtension + if err := json.Unmarshal(b, &out); err != nil { + return nil, fmt.Errorf("unmarshal extensions: %w", err) + } + return out, nil +}api/spec/src/errors.tsp (1)
55-65: Align example with ValidationError shape (includefield).Minor consistency tweak to avoid confusing examples.
Apply:
- @example(#{ - validationErrors: #[ - #{ code: "validation_error", message: "Validation error" } - ], - otherAttribute: "otherValue", - }) + @example(#{ + validationErrors: #[ + #{ field: "path.to.field", code: "validation_error", message: "Validation error" } + ], + otherAttribute: "otherValue", + })api/spec/Makefile (1)
20-22: Tighten AWK stop condition to avoid dropping unintended paths
Stop skipping on the next path key (two-space indent + slash) or oncomponents:to prevent removing real paths after/prune-this-in-script::hack: ## Hack the OpenAPI spec - awk 'BEGIN{skip=0} /^[[:space:]]*\/prune-this-in-script:/ {skip=1; next} skip && /^components:/ {skip=0} !skip' ../openapi.yaml > ../openapi.yaml.tmp && mv ../openapi.yaml.tmp ../openapi.yaml + awk 'BEGIN{skip=0} /^[[:space:]]*\/prune-this-in-script:/ {skip=1; next} skip && (/^[[:space:]]{2}\/[^[:space:]]+:/ || /^components:/) {skip=0} !skip' ../openapi.yaml > ../openapi.yaml.tmp && mv ../openapi.yaml.tmp ../openapi.yaml - awk 'BEGIN{skip=0} /^[[:space:]]*\/prune-this-in-script:/ {skip=1; next} skip && /^components:/ {skip=0} !skip' ../openapi.cloud.yaml > ../openapi.cloud.yaml.tmp && mv ../openapi.cloud.yaml.tmp ../openapi.cloud.yaml + awk 'BEGIN{skip=0} /^[[:space:]]*\/prune-this-in-script:/ {skip=1; next} skip && (/^[[:space:]]{2}\/[^[:space:]]+:/ || /^components:/) {skip=0} !skip' ../openapi.cloud.yaml > ../openapi.cloud.yaml.tmp && mv ../openapi.cloud.yaml.tmp ../openapi.cloud.yamlapi/openapi.cloud.yaml (1)
11878-11895: Internal /types endpoint: add op-level x-internal and consider clearer operationId to prevent SDK exposureSome generators ignore path-level
x-internal. Add it at operation level too and use a clearer, unique operationId to avoid accidental inclusion in clients.Proposed tweak:
/types: get: - operationId: Exports_exports + operationId: Types_exportTreeShaken + x-internal: true summary: Exports tree shaken types we want to export which are then hidden from the API docs. This API doesn't actually exist or return anything, it's just used for exporting tree-shaken types through codegeneration and SDKsAlso verify your doc/codegen toolchain honors
x-internalfor both docs and SDK generation. If not, add the appropriate vendor extension (e.g., an SDK‑specific ignore flag) used by your generators.api/client/javascript/src/client/schemas.ts (1)
5628-5647: Make additional properties readonly; confirmfieldpresence across implementations
- For consistency with readonly core fields, make the index signature readonly.
- Confirm that
fieldis always present (it’s required here and in Go structs), otherwise consider making it optional.Apply:
} & { - [key: string]: unknown + readonly [key: string]: unknown }Note: Go clients define
ErrorExtension.Field string(non-pointer, noomitempty), so the JSON field is always emitted. Please verify server behavior matches this.api/openapi.yaml (1)
9605-9609: Centralize 400 Bad Request response into components/responses
Define a newcomponents/responses/BadRequestProblemthat references#/components/schemas/BadRequestProblemResponse, then replace all inline 400 definitions with:responses: '400': $ref: '#/components/responses/BadRequestProblem'Since
BadRequestProblemResponsealready inheritsextensionsfromUnexpectedProblemResponse, no schema changes are required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
api/client/go/error.go(2 hunks)api/client/javascript/src/client/schemas.ts(12 hunks)api/openapi.cloud.yaml(10 hunks)api/openapi.yaml(9 hunks)api/spec/Makefile(1 hunks)api/spec/src/cloud/main.tsp(1 hunks)api/spec/src/errors.tsp(2 hunks)api/spec/src/main.tsp(1 hunks)api/spec/src/productcatalog/errors.tsp(1 hunks)api/spec/src/productcatalog/routes.tsp(7 hunks)e2e/productcatalog_test.go(3 hunks)openmeter/productcatalog/errors.go(47 hunks)openmeter/productcatalog/planaddon_test.go(6 hunks)openmeter/productcatalog/subscription/http/errors.go(2 hunks)openmeter/server/framework_test.go(1 hunks)openmeter/subscription/errors.go(10 hunks)openmeter/subscription/errors_test.go(10 hunks)pkg/framework/commonhttp/errors.go(2 hunks)pkg/framework/commonhttp/errors_test.go(1 hunks)pkg/models/testutils.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-04-21T08:32:31.689Z
Learnt from: chrisgacsal
PR: openmeterio/openmeter#2699
File: openmeter/productcatalog/planaddon/service/service_test.go:210-211
Timestamp: 2025-04-21T08:32:31.689Z
Learning: In `productcatalog.UsageBasedRateCard`, the `BillingCadence` field is a non-pointer `isodate.Period`, while in `productcatalog.FlatFeeRateCard`, `BillingCadence` is a pointer type (`*isodate.Period`). This means `MonthPeriod` should be used directly for `UsageBasedRateCard` (not `&MonthPeriod`).
Applied to files:
openmeter/productcatalog/errors.go
🧬 Code graph analysis (10)
api/client/go/error.go (3)
api/client/javascript/src/client/schemas.ts (1)
ErrorExtension(11652-11652)api/api.gen.go (1)
ErrorExtension(3422-3432)api/client/go/client.gen.go (1)
ErrorExtension(3136-3146)
openmeter/subscription/errors.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
api/client/javascript/src/client/schemas.ts (3)
pkg/models/validationissue.go (1)
ErrorExtension(11-11)api/api.gen.go (1)
ErrorExtension(3422-3432)api/client/go/client.gen.go (1)
ErrorExtension(3136-3146)
openmeter/productcatalog/errors.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
pkg/framework/commonhttp/errors.go (3)
pkg/models/validationissue.go (1)
ValidationIssue(18-25)api/api.gen.go (1)
ValidationIssue(7521-7551)pkg/models/attributes.go (1)
Attributes(8-8)
pkg/framework/commonhttp/errors_test.go (3)
pkg/models/errors.go (1)
ErrorCode(294-294)pkg/models/validationissue.go (1)
NewValidationIssue(273-283)pkg/framework/commonhttp/errors.go (2)
WithHTTPStatusCodeAttribute(83-85)HandleIssueIfHTTPStatusKnown(114-196)
e2e/productcatalog_test.go (1)
api/client/go/error.go (1)
ParseExtensionsFromInterface(20-30)
openmeter/productcatalog/planaddon_test.go (3)
pkg/models/validationissue.go (2)
NewValidationIssue(273-283)WithField(216-222)openmeter/productcatalog/errors.go (6)
ErrPlanAddonIncompatibleStatus(19-25)ErrPlanAddonMaxQuantityMustNotBeSet(39-45)ErrPlanAddonCurrencyMismatch(49-55)ErrRateCardFeatureKeyMismatch(116-122)ErrRateCardBillingCadenceMismatch(126-132)ErrRateCardEntitlementTemplateTypeMismatch(136-142)pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
openmeter/subscription/errors_test.go (1)
pkg/framework/commonhttp/errors.go (1)
WithHTTPStatusCodeAttribute(83-85)
openmeter/productcatalog/subscription/http/errors.go (3)
pkg/models/validationissue.go (2)
AsValidationIssues(331-342)ValidationIssues(295-295)pkg/framework/commonhttp/errors.go (1)
HandleIssueIfHTTPStatusKnown(114-196)openmeter/subscription/fielddescriptormapper.go (1)
MapSubscriptionSpecValidationIssueField(19-53)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: Migration Checks
- GitHub Check: Code Generators
- GitHub Check: Test
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (18)
openmeter/productcatalog/subscription/http/errors.go (1)
55-68: LGTM!The helper function is well-structured with clear separation of concerns. Preallocating the result slice capacity is good for performance, and the error handling correctly returns early on mapping failures. The function signature and implementation align well with the PR's goal of centralizing validation issue mapping.
</review_comment_end -->
pkg/framework/commonhttp/errors_test.go (1)
15-50: LGTM!The test effectively validates that:
- HTTP 400 status is correctly mapped from the validation issue attribute
- The response uses
validationErrorskey (notvalidationIssues)- Internal attributes (HTTP status code) are stripped from the response
- Public attributes (code, message, severity) are preserved
The test structure is clear and aligns with the PR objective of preventing private error details from leaking via API responses.
api/spec/src/productcatalog/errors.tsp (1)
10-17: LGTM!The alias rename and union type updates align with the PR's error-extension standardization:
CommonSubscriptionErrorsmore accurately reflects the scopeBadRequestErrorreplacesValidationErrorResponsefor consistency- Additional error types expand the error surface appropriately
pkg/models/testutils.go (1)
24-25: LGTM!The comment clarification is helpful and accurately reflects that the full attribute set (including HTTP status) should be compared in tests without stripping.
openmeter/productcatalog/errors.go (2)
5-11: LGTM!The import additions are necessary for the HTTP status attribute changes throughout the file.
19-574: LGTM! HTTP status attributes added consistently.The systematic addition of
commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest)to validation errors enables proper HTTP 400 error mapping. The pattern is applied consistently across all user-facing validation errors in the product catalog domain.The errors that don't receive the attribute (e.g.,
ErrPlanAddonUnknownPlanPhaseKey,ErrRateCardKeyMismatch) appear to be intentionally excluded, likely because they're internal or need different handling.openmeter/server/framework_test.go (1)
22-71: LGTM!The test correctly validates that 400 Bad Request errors expose validation issues under the
validationErrorskey (renamed fromvalidationIssuesfor consistency). The test also confirms that internal attributes like HTTP status codes are properly stripped from the response.pkg/framework/commonhttp/errors.go (3)
141-141: LGTM!Grammar improvement in the log message.
168-172: LGTM!The key rename from
validationIssuestovalidationErrorsand comment update properly reflect the standardized error extension naming convention for 400 responses.
175-183: Important: Prevents leaking error codes as attributes.The addition of
delete(attrs, issue.Code())on line 179 ensures that the error code is not exposed as an attribute in the response, complementing the removal of the HTTP status attribute. This aligns with the PR objective of not leaking private error details.api/spec/src/cloud/main.tsp (1)
118-120: LGTM!The new
Exportsinterface provides an internal/typesendpoint for exporting tree-shaken types to support client code generation. Thex-internalannotation correctly marks this as a tooling endpoint not intended for public API consumption.openmeter/subscription/errors_test.go (2)
5-5: LGTM!The import additions are necessary for the HTTP status attribute assertions added throughout the test file.
Also applies to: 17-17
205-338: LGTM!The test expectations are correctly updated to include
commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest)for all validation issues. This ensures that the tests verify that HTTP status attributes are properly carried through the validation flow, both in unmapped and mapped field selector scenarios.The consistent application of the attribute across all expected issues aligns with the PR's objective of standardizing HTTP error handling.
openmeter/productcatalog/planaddon_test.go (1)
359-387: LGTM: status attributes are now carried on validation issues.Good use of commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest) to ensure automatic HTTP mapping.
Also applies to: 565-592
openmeter/subscription/errors.go (1)
23-23: LGTM! Consistent HTTP status code attributes added to validation errors.The addition of
commonhttp.WithHTTPStatusCodeAttribute(http.StatusBadRequest)to these validation errors is consistent and appropriate. The pattern follows the existing usage in the file (e.g., lines 39 and 49 for conflict errors with 409 status), and HTTP 400 is the correct status code for validation failures.Also applies to: 92-92, 103-103, 112-112, 121-121, 131-131, 142-142, 151-151, 160-160, 169-169
api/client/javascript/src/client/schemas.ts (2)
11652-11652: LGTM: publicErrorExtensionexportConsistent with server and Go client; enables SDKs to model validation issues.
2438-2454: Internal/typespath is already excluded from JS SDK exports (flaggedx-internal; no callable or runtime function generated)api/spec/src/productcatalog/routes.tsp (1)
489-489: LGTM! Consistent error type refinement.All subscription mutation operations now use
CommonSubscriptionErrorsinstead of the genericOpenMeter.CommonErrors, providing more specific error handling for subscription-related operations. This aligns with the PR's goal of properly handling ErrorExtension conversion and validation errors.Also applies to: 509-509, 528-528, 571-571, 606-606, 619-619, 630-630
| /** | ||
| * @description Additional properties specific to the problem type may be present. | ||
| * @example { | ||
| * "validationErrors": [ | ||
| * { | ||
| * "code": "validation_error", | ||
| * "message": "Validation error" | ||
| * } | ||
| * ], | ||
| * "otherAttribute": "otherValue" | ||
| * } | ||
| */ | ||
| extensions?: { | ||
| [key: string]: unknown | ||
| } |
There was a problem hiding this comment.
Doc example for extensions.validationErrors contradicts ErrorExtension (missing field)
The example objects omit field, but ErrorExtension requires it. Fix the example (or relax the schema, if truly optional).
Apply:
* @example {
* "validationErrors": [
* {
- * "code": "validation_error",
- * "message": "Validation error"
+ * "field": "subscription.spec.plan",
+ * "code": "validation_error",
+ * "message": "Validation error"
* }
* ],
* "otherAttribute": "otherValue"
* }Optional: make the index signature readonly for parity:
- extensions?: {
- [key: string]: unknown
- }
+ extensions?: {
+ readonly [key: string]: unknown
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * @description Additional properties specific to the problem type may be present. | |
| * @example { | |
| * "validationErrors": [ | |
| * { | |
| * "code": "validation_error", | |
| * "message": "Validation error" | |
| * } | |
| * ], | |
| * "otherAttribute": "otherValue" | |
| * } | |
| */ | |
| extensions?: { | |
| [key: string]: unknown | |
| } | |
| /** | |
| * @description Additional properties specific to the problem type may be present. | |
| * @example { | |
| * "validationErrors": [ | |
| * { | |
| * "field": "subscription.spec.plan", | |
| * "code": "validation_error", | |
| * "message": "Validation error" | |
| * } | |
| * ], | |
| * "otherAttribute": "otherValue" | |
| * } | |
| */ | |
| extensions?: { | |
| readonly [key: string]: unknown | |
| } |
🤖 Prompt for AI Agents
In api/client/javascript/src/client/schemas.ts around lines 10974 to 10988, the
JSDoc example for extensions.validationErrors omits the required field property
that ErrorExtension mandates; update the example objects to include a field
(e.g., "field": "name") so the example matches the ErrorExtension schema, or if
field is meant to be optional, relax the ErrorExtension type accordingly;
additionally, consider making the extensions index signature readonly (readonly
[key: string]: unknown) for parity.
| /** @description The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing). */ | ||
| 400: { | ||
| headers: { | ||
| [name: string]: unknown | ||
| } | ||
| content: { | ||
| 'application/problem+json': components['schemas']['ValidationErrorProblemResponse'] | ||
| 'application/problem+json': components['schemas']['BadRequestProblemResponse'] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Switch to application/problem+json: verify runtime mapping/parsing
Standardizing error responses is good. Ensure the JS client runtime matches on application/problem+json for 4xx/5xx, otherwise errors might not parse.
Run:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm problem+json appears in generated client types
rg -n -C2 'application/problem\\+json' --type ts || true
# Inspect runtime/consumption code for MIME matching
rg -n -C3 -e 'problem\\+json' -e 'isJsonMime' -e 'application/json' --type ts || true
# Spot mappings for 400/401/403/404/412/500/503 to problem+json
rg -nPU -C2 '(?s)\\b(400|401|403|404|412|500|503)\\b.*application/problem\\+json' --type ts || trueLength of output: 136036
Add application/problem+json mapping for error responses: extend the JS client’s runtime to recognize and parse application/problem+json for all 4xx/5xx responses (currently only application/json is supported).
🤖 Prompt for AI Agents
In api/client/javascript/src/client/schemas.ts around lines 24269-24276, the
client only maps error responses to application/json; update the runtime
response mappings so that all 4xx and 5xx responses also recognize
'application/problem+json' and parse them into the existing
BadRequestProblemResponse / ProblemResponse schemas: add
'application/problem+json' to the content map for each error status, update the
response deserializer switch to accept that media type (falling back to
application/json if needed), and ensure the generated types include the
problem+json schema so callers can access the parsed ProblemResponse object.
| description: The server cannot or will not process the request due to something that is perceived to be a client error (e.g., malformed request syntax, invalid request message framing, or deceptive request routing). | ||
| content: | ||
| application/problem+json: | ||
| schema: | ||
| $ref: '#/components/schemas/ValidationErrorProblemResponse' | ||
| $ref: '#/components/schemas/BadRequestProblemResponse' |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Standardized 400 problem+json to BadRequestProblemResponse — looks right; please verify component presence and shape
Good move to consolidate 400 responses under a single schema. Ensure #/components/schemas/BadRequestProblemResponse exists and contains only safe, public fields (no private/internal details).
Run this scan to confirm the definition and review its block:
Also applies to: 9759-9763, 9836-9840, 10220-10224, 10309-10313, 10394-10398, 10574-10578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the BadRequestProblemResponse schema (or response) definition
rg -n -C2 -e 'BadRequestProblemResponse:' -e '#/components/(schemas|responses)/BadRequestProblemResponse' --type yaml
# Show the full enclosing component block for manual inspection
rg -nPU '(?s)^\s*(schemas|responses):\s*$.*?^\s*[A-Za-z]|\z' --type yaml | sed -n '/BadRequestProblemResponse:/,/^\s*[A-Za-z]/p'Length of output: 123714
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show BadRequestProblemResponse definition
echo "=== BadRequestProblemResponse schema ==="
rg -n -C5 '^ *BadRequestProblemResponse:' api/openapi.cloud.yaml
sed -n '13217,13300p' api/openapi.cloud.yaml
# 2) Locate /types path, x-internal and operationId
echo "=== /types path snippet ==="
rg -n -C5 "'/types':" api/openapi.cloud.yaml
rg -n -C3 'x-internal: true' api/openapi.cloud.yaml
# 3) Show ErrorExtension schema definition
echo "=== ErrorExtension schema ==="
rg -n -C5 '^ *ErrorExtension:' api/openapi.cloud.yaml
rg -n -C5 '#/components/schemas/ErrorExtension' api/openapi.cloud.yamlLength of output: 3708
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate /types path ==="
rg -n -C5 "^ *['\"]?/types['\"]?:\s*$" api/openapi.cloud.yaml || echo "/types path not found"
echo
echo "=== Locate ErrorExtension schema ==="
rg -n "ErrorExtension" api/openapi.cloud.yaml -C2
# If found, dump the first ErrorExtension block
line=$(rg -n "^ *ErrorExtension:" -m1 api/openapi.cloud.yaml | cut -d: -f1 || echo "")
if [ -n "$line" ]; then
echo
echo "=== ErrorExtension block (lines $line–$((line+20))) ==="
sed -n "${line},$((line+20))p" api/openapi.cloud.yaml
else
echo "ErrorExtension definition not found"
fiLength of output: 1989
Update /types internal endpoint and tighten ErrorExtension schema
/types(api/openapi.cloud.yaml:11878): addx-internal: trueat the path or operation level and renameoperationId: Exports_exportsto a conventional name (e.g.exportTypes).ErrorExtensionschema (api/openapi.cloud.yaml:16955): changeadditionalProperties: {}toadditionalProperties: false, update its description to referenceHTTPProblem.extensions, and adjust any examples to include the requiredfieldproperty.
🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 11878 and 16955, update two items: add
x-internal: true at the /types path or its operation level and rename the
operationId from Exports_exports to a conventional name such as exportTypes
(ensure any references to the old operationId are updated); and for the
ErrorExtension schema at line ~16955 set additionalProperties: false, update the
schema description to reference HTTPProblem.extensions, and modify examples to
include the required field property so they conform to the tightened schema.
| ErrorExtension: | ||
| type: object | ||
| required: | ||
| - field | ||
| - code | ||
| - message | ||
| properties: | ||
| field: | ||
| type: string | ||
| description: The path to the field. | ||
| example: addons/pro/ratecards/token/featureKey | ||
| readOnly: true | ||
| code: | ||
| type: string | ||
| description: The machine readable description of the error. | ||
| example: invalid_feature_key | ||
| readOnly: true | ||
| message: | ||
| type: string | ||
| description: The human readable description of the error. | ||
| example: not found feature by key | ||
| readOnly: true | ||
| additionalProperties: {} | ||
| description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode] | ||
| Event: |
There was a problem hiding this comment.
Tighten ErrorExtension contract and fix description inconsistency
- Make the schema closed to avoid accidental extra fields.
- Align description with how it’s used (extensions.validationErrors items), not “Extensions.[StatusCode]”.
Apply:
ErrorExtension:
type: object
required:
- field
- code
- message
properties:
@@
- additionalProperties: {}
- description: Generic ErrorExtension as part of HTTPProblem.Extensions.[StatusCode]
+ additionalProperties: false
+ description: Item shape used in HTTPProblem.extensions.validationErrors[]🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 16955-16979, the ErrorExtension schema is
currently open (additionalProperties: {}) and its description wrongly references
"HTTPProblem.Extensions.[StatusCode]"; change additionalProperties to false to
make the schema closed and update the description to state that this is the
generic ErrorExtension used for items in extensions.validationErrors (e.g.,
"Generic ErrorExtension used for items in extensions.validationErrors"). Ensure
required/readonly fields remain unchanged.
| extensions: | ||
| type: object | ||
| additionalProperties: {} | ||
| description: Additional properties specific to the problem type may be present. | ||
| example: | ||
| validationErrors: | ||
| - code: validation_error | ||
| message: Validation error | ||
| otherAttribute: otherValue |
There was a problem hiding this comment.
Example inconsistency: include field in validationErrors items or relax ErrorExtension.required
Your example shows items with code and message only, but ErrorExtension requires field. Either add field to the example or make field optional if it isn’t always present.
Suggested example fix:
example:
validationErrors:
- - code: validation_error
- message: Validation error
+ - field: addons/pro/ratecards/token/featureKey
+ code: validation_error
+ message: Validation error
otherAttribute: otherValue📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extensions: | |
| type: object | |
| additionalProperties: {} | |
| description: Additional properties specific to the problem type may be present. | |
| example: | |
| validationErrors: | |
| - code: validation_error | |
| message: Validation error | |
| otherAttribute: otherValue | |
| extensions: | |
| type: object | |
| additionalProperties: {} | |
| description: Additional properties specific to the problem type may be present. | |
| example: | |
| validationErrors: | |
| - field: addons/pro/ratecards/token/featureKey | |
| code: validation_error | |
| message: Validation error | |
| otherAttribute: otherValue |
🤖 Prompt for AI Agents
In api/openapi.cloud.yaml around lines 24158 to 24166, the example for
extensions.validationErrors omits the required property "field" defined by
ErrorExtension; update the example to include a "field" value for each
validationErrors item (e.g., field: "attributeName") so it matches the schema,
or if "field" is not always present, modify the ErrorExtension schema by
removing "field" from its required list (or mark it nullable/optional) and
update any related documentation accordingly.
| extensions: | ||
| type: object | ||
| additionalProperties: {} | ||
| description: Additional properties specific to the problem type may be present. | ||
| example: | ||
| validationErrors: | ||
| - code: validation_error | ||
| message: Validation error | ||
| otherAttribute: otherValue |
There was a problem hiding this comment.
🧩 Analysis chain
extensions example violates ErrorExtension.required and schema is too permissive; propose constrained, typed schema.
The example item misses required 'field'. Also additionalProperties: {} allows arbitrary keys, risking private detail leakage. Define validationErrors as an array of ErrorExtension and disallow other keys.
Apply this diff:
extensions:
type: object
- additionalProperties: {}
+ additionalProperties: false
+ properties:
+ validationErrors:
+ type: array
+ items:
+ $ref: '#/components/schemas/ErrorExtension'
description: Additional properties specific to the problem type may be present.
example:
- validationErrors:
- - code: validation_error
- message: Validation error
- otherAttribute: otherValue
+ validationErrors:
+ - field: addons/pro/ratecards/token/featureKey
+ code: validation_error
+ message: Validation errorIf you intentionally need other extension keys, explicitly whitelist them here instead of keeping additionalProperties open.
🏁 Script executed:
#!/bin/bash
# Verify that no other examples of extensions contradict the tightened schema
rg -n -C2 'extensions:\s*\n\s*type:\s*object' api/openapi.yaml
# Verify ErrorExtension is referenced by validationErrors after changes
rg -n -C2 'validationErrors|ErrorExtension' api/openapi.yamlLength of output: 2377
Revise verification script for extensions schemas
Please run the following to enumerate all extensions: definitions and examples, and confirm ErrorExtension usage:
🏁 Script executed:
#!/bin/bash
# List each 'extensions:' schema block with its additionalProperties setting
rg -n '^\s*extensions:' -A4 api/openapi.yaml | grep -n -B2 'additionalProperties'
# Show all example blocks under extensions
rg -n '^\s*extensions:' -A10 api/openapi.yaml | grep -n -C1 'example:'
# Confirm all validationErrors items reference ErrorExtension
rg -n -C2 'validationErrors:' api/openapi.yaml
rg -n -C2 'ErrorExtension' api/openapi.yamlLength of output: 1847
Tighten extensions schema & correct example
In api/openapi.yaml (lines 25191–25199):
extensions:
type: object
- additionalProperties: {}
+ additionalProperties: false
+ properties:
+ validationErrors:
+ type: array
+ items:
+ $ref: '#/components/schemas/ErrorExtension'
description: Additional properties specific to the problem type may be present.
example:
- validationErrors:
- - code: validation_error
- message: Validation error
- otherAttribute: otherValue
+ validationErrors:
+ - field: addons/pro/ratecards/token/featureKey
+ code: validation_error
+ message: Validation errorThis enforces ErrorExtension.required and disallows arbitrary keys.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| extensions: | |
| type: object | |
| additionalProperties: {} | |
| description: Additional properties specific to the problem type may be present. | |
| example: | |
| validationErrors: | |
| - code: validation_error | |
| message: Validation error | |
| otherAttribute: otherValue | |
| extensions: | |
| type: object | |
| additionalProperties: false | |
| properties: | |
| validationErrors: | |
| type: array | |
| items: | |
| $ref: '#/components/schemas/ErrorExtension' | |
| description: Additional properties specific to the problem type may be present. | |
| example: | |
| validationErrors: | |
| - field: addons/pro/ratecards/token/featureKey | |
| code: validation_error | |
| message: Validation error |
🤖 Prompt for AI Agents
In api/openapi.yaml around lines 25191 to 25199, the `extensions` schema
currently allows arbitrary keys and the example includes a disallowed
`otherAttribute`; change the schema to disallow extra properties (set
additionalProperties: false) and add a `required` list to enforce the expected
extension fields (e.g., required: ["validationErrors"] or the actual required
extension keys used by ErrorExtension), and update the example to only include
the allowed fields (remove `otherAttribute`) and ensure `validationErrors`
matches the declared schema shape.
| /** | ||
| * This interface can be used to export tree shaken types into OAPI schema. | ||
| * A Makefile script will delete lines of this interface from the OpenAPI spec. | ||
| */ | ||
| @extension("x-internal", true) | ||
| @route("/prune-this-in-script") | ||
| interface Exports { | ||
| @summary("Exports tree shaken types we want to export which are then hidden from the API docs. This API doesn't actually exist or return anything, it's just used for exporting tree-shaken types through codegeneration and SDKs") | ||
| @extension("x-internal", true) | ||
| exports(): { | ||
| @body body: { | ||
| errorExtension: ErrorExtension; | ||
| }; | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the Makefile script properly prunes this interface from the OpenAPI spec.
The placeholder route /prune-this-in-script and the x-internal extensions rely on post-processing to remove this interface from the final OpenAPI specification. Please ensure that:
- The Makefile script referenced in the comments exists and correctly prunes all traces of this interface
- The
x-internal: trueextension is properly respected by all code generation tools
Run the following script to verify the Makefile and check if the route appears in generated OpenAPI specs:
🏁 Script executed:
#!/bin/bash
# Description: Verify Makefile pruning script exists and check generated OpenAPI specs
# Check if Makefile exists and contains pruning logic
if [ -f "api/spec/Makefile" ]; then
echo "=== Makefile content related to pruning ==="
cat api/spec/Makefile | grep -A 5 -B 5 -i "prune"
else
echo "ERROR: api/spec/Makefile not found"
fi
echo -e "\n=== Check if /prune-this-in-script appears in OpenAPI specs ==="
rg -n "/prune-this-in-script" api/*.yaml api/*.json 2>/dev/null || echo "Route not found in OpenAPI specs (expected after pruning)"
echo -e "\n=== Check if Exports interface appears in OpenAPI specs ==="
rg -n -A 3 "Exports" api/*.yaml api/*.json 2>/dev/null || echo "Exports not found in OpenAPI specs (expected after pruning)"Length of output: 1884
Fix hack target to fully prune /prune-this-in-script
The current awk rule only removes the path key but leaves its nested operationId: Exports_exports and response definitions. Adjust the skip logic in the hack target to cover the entire operation block (e.g., through the next top-level path or end of the paths: section). [Makefile: hack]
🤖 Prompt for AI Agents
In api/spec/src/main.tsp around lines 114 to 128, the Makefile "hack" target's
awk removal only deletes the "/prune-this-in-script" path key but leaves its
nested operationId and response definitions; update the hack target so the awk
(or sed) skip logic removes the entire operation block by skipping from the
matched path line through the next top-level path entry or the end of the paths:
section — implement this by adjusting the pattern to detect the start of the
path (e.g., a line matching two spaces then "/prune-this-in-script") and then
suppress output until a line that matches a top-level path key (e.g., two spaces
followed by "/" and non-space) or the "paths:" section end is seen, ensuring all
nested indented lines for that operation are excluded.
| // We can't simply type assert as it will panic on interface{} so let's convert it manually | ||
| validationErrors := apiRes.ApplicationproblemJSON409.Extensions["409"].([]any) | ||
| extesions, err := api.ParseExtensionsFromInterface(validationErrors) | ||
| require.Nil(t, err) | ||
| require.NotNil(t, extesions) | ||
| require.GreaterOrEqual(t, len(extesions), 1) | ||
|
|
||
| valErr := extesions[0] | ||
| require.NotNil(t, valErr) | ||
| require.Equal(t, "only_single_subscription_allowed_per_customer_at_a_time", valErr.Code, "received the following body: %s", apiRes.Body) | ||
| require.NotContains(t, valErr.AdditionalProperties, "models.ErrorCode:only_single_subscription_allowed_per_customer_at_a_time") | ||
| }) |
There was a problem hiding this comment.
Remove unsafe type assertion; use the parsing helper directly.
Avoid .( []any ) which can panic. Pass the interface directly to ParseExtensionsFromInterface.
Apply:
- // We can't simply type assert as it will panic on interface{} so let's convert it manually
- validationErrors := apiRes.ApplicationproblemJSON409.Extensions["409"].([]any)
- extesions, err := api.ParseExtensionsFromInterface(validationErrors)
- require.Nil(t, err)
- require.NotNil(t, extesions)
- require.GreaterOrEqual(t, len(extesions), 1)
-
- valErr := extesions[0]
+ exts, err := api.ParseExtensionsFromInterface(apiRes.ApplicationproblemJSON409.Extensions["409"])
+ require.NoError(t, err)
+ require.NotEmpty(t, exts)
+ valErr := exts[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // We can't simply type assert as it will panic on interface{} so let's convert it manually | |
| validationErrors := apiRes.ApplicationproblemJSON409.Extensions["409"].([]any) | |
| extesions, err := api.ParseExtensionsFromInterface(validationErrors) | |
| require.Nil(t, err) | |
| require.NotNil(t, extesions) | |
| require.GreaterOrEqual(t, len(extesions), 1) | |
| valErr := extesions[0] | |
| require.NotNil(t, valErr) | |
| require.Equal(t, "only_single_subscription_allowed_per_customer_at_a_time", valErr.Code, "received the following body: %s", apiRes.Body) | |
| require.NotContains(t, valErr.AdditionalProperties, "models.ErrorCode:only_single_subscription_allowed_per_customer_at_a_time") | |
| }) | |
| exts, err := api.ParseExtensionsFromInterface(apiRes.ApplicationproblemJSON409.Extensions["409"]) | |
| require.NoError(t, err) | |
| require.NotEmpty(t, exts) | |
| valErr := exts[0] | |
| require.NotNil(t, valErr) | |
| require.Equal(t, "only_single_subscription_allowed_per_customer_at_a_time", valErr.Code, "received the following body: %s", apiRes.Body) | |
| require.NotContains(t, valErr.AdditionalProperties, "models.ErrorCode:only_single_subscription_allowed_per_customer_at_a_time") | |
| }) |
🤖 Prompt for AI Agents
In e2e/productcatalog_test.go around lines 581 to 592, the code performs an
unsafe type assertion to []any on
apiRes.ApplicationproblemJSON409.Extensions["409"] which can panic; instead pass
the raw interface value from the Extensions map directly into
api.ParseExtensionsFromInterface, remove the manual conversion and related
variables, check the returned error and result as before, and then assert on
extesions[0] as done currently.
2 participants
Overview
We leaked error details through the API response that were intended to be private, the scope of this patch is to generally solve that.
ErrorExtensionconversion:HTTP.Problem.Extentensionscan have specific flavors (e.g.ValidationErrorused inproductcatalog) but the general typing of it in TypeSpec was incorrect.api/spec/Makefile)ValidationErrorencoder in subscription http drivercommonhttp.WithHTTPStatusCodeAttributeto productcatalog errors so they are automatically mappedValidationIssue.Fieldmapping (SubscriptionSpectoapi.SubscriptionViewpath structures, uses newFieldDescriptor.Attributes)Summary by CodeRabbit
New Features
Bug Fixes
Tests