fix(openapi)!: remove name from ConfigUser request schema#157
Open
Dav-14 wants to merge 1 commit into
Open
Conversation
The published openapi.yaml declared an optional `name` property on `ConfigUser` (the insertConfig / updateConfig request body). The server-side Go struct never had a `Name` field — only the response type `Config` does — and `decodeJSONBody` runs with `json.Decoder.DisallowUnknownFields()` (pkg/server/helpers.go). Net effect: any client honouring the spec and sending `name` got back `400 VALIDATION — Request body contains unknown field "name"` without the config being created. Formance's terraform-hcp-proxy hits this in production whenever a manifest declares a webhooks config (the manifest's natural key was being marshalled into `name`). This commit aligns the spec with what the server actually accepts: drop `name` from `ConfigUser`. The `Config` response schema was not exposing `name` either, so the response shape is unchanged. SDK impact: the Speakeasy-generated `pkg/client` is regenerated. `ConfigUser.Name` and `ConfigUser.GetName()` are gone. Anyone relying on them was already getting silent 400s at runtime, so the surface change is the only thing that changes. BREAKING CHANGE: ConfigUser.Name and the corresponding accessor are removed from the Go SDK. Existing call sites must drop the field.
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (56)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openapi.yamladvertises an optionalnameproperty onConfigUser(used byPOST /configsandPUT /configs/{id}), but the server-side Go structwebhooks.ConfigUser(pkg/config.go) only declaresEndpoint,Secret,EventTypes. Server-side decoding runs withjson.Decoder.DisallowUnknownFields()(pkg/server/helpers.go:18), so any client honouring the spec and sendingnameis rejected with400 VALIDATION — Request body contains unknown field "name"and no config is created.terraform-hcp-proxyhits this in production whenever a manifest declares a webhooks config: the manifest's natural key was being marshalled intoname, dispatch failed, deployment wenterrored.namefromConfigUser. TheConfigresponse schema (WebhooksConfig) was not exposingnameeither, so response shape is unchanged.History — has
nameever worked?No.
namefirst appeared inopenapi.yamlin commita16b8397(Jan 17, 2024) —feat(webhooks): ENG-459 give optional names to webhook configs (#1123). That same commit:namecolumn viacmd/migrate.go(still in the schema today)Name string \json:"name" bun:"name,nullzero"`to the response/storage structConfig`name:to the request schemaConfigUserinopenapi.yamlNameto the request Go structConfigUsercfg.Name = …git grepacrossv2.3.2confirms no code anywhere outside the generated SDK references.Nameon aConfigUser. Net effect since Jan 2024:namerejected byDisallowUnknownFields()— has never workedConfigGo struct marshals"name":""because of the json tag — raw clients see an empty stringWebhooksConfigdoes not exposename— SDK consumers never see it eitherSo this PR removes a spec promise that the implementation has never made good on. SDK consumers using
ConfigUser.Namewere getting silent 400s in production.Why this PR is large
Most of the diff is Speakeasy SDK regen against a slightly older baseline (
gen.lockwas several feature versions behind). Reviewer focus should be onopenapi.yaml(4 deleted lines); the rest is mechanical Speakeasy output. Happy to split the SDK regen out if preferred, but they need to land together to keep the SDK consistent with the spec.BREAKING CHANGE
pkg/client/models/components.ConfigUserlosesName *stringandGetName(). Call sites must drop the field. The field was non-functional at runtime, so removal has no behavioural impact — only a compile-time SDK shape change.Follow-ups (out of scope)
namecolumn. Drop it in a follow-up migration if appropriate.feat:PR — needsNameon the request struct, a handler change, and the response schema to expose it.Test plan
just generate-client— Speakeasy regen succeeded; newConfigUserhas onlyEndpoint,Secret,EventTypes.go build ./...— clean.go test ./pkg/...— all pass (pkg,pkg/backoff,pkg/storage/postgres,pkg/worker).-tags it) end-to-end.