diff --git a/docs/contract.md b/docs/contract.md index 457238f..a68734c 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -166,7 +166,7 @@ custom type or collection kind that behaviour calls for. |---|---|---| | Omitted entirely | The type cannot be represented | `deriveType` marks it unsupported | | `Required` | Writable and required on create | the create body's `required` | -| `Optional` + `Computed` | Writable, and the response carries a value whether or not the request supplied one | `x-tfpfgen-server-default`, or the response schema's `required` | +| `Optional` + `Computed` | Writable, and the response carries a value whether or not the request supplied one | `x-tfpfgen-server-default`, the response schema's `required`, or a `default` on the request property | | `Computed` | The practitioner cannot set it | absent from the create body, `readOnly`, `x-tfpfgen-server-forced`, `x-tfpfgen-volatile` | | `Optional` | Writable, and the server leaves it absent when omitted | none of the above | @@ -175,14 +175,29 @@ they accept, so a writable attribute usually belongs in `Optional` + `Computed`; emitting it as `Optional` alone gives the practitioner a perpetual diff, because Terraform holds null in config against a value in state. -That is why `x-tfpfgen-server-default` exists rather than OpenAPI's `default`. -The response schema's `required` list was the only route to `Optional` + -`Computed`, and a document that declares nothing required in its responses — as -real ones do — sends every writable attribute to plain `Optional`. The audit -measures the same fact by omitting the attribute and reading what comes back, -and the extension is where that reading is recorded. OpenAPI's `default` says -what the document declares, is read by nothing in the generation path, and on a -`$ref`'d property is written onto a schema every other use of that type shares. +Three declarations reach `Optional` + `Computed`, of decreasing authority. + +`x-tfpfgen-server-default` is the audit's own measurement, taken by omitting the +attribute and reading what comes back. It is the only one that does not depend +on the document being diligent, which is why it exists. + +The response schema's `required` list is the document asserting the same fact. +It is too weak on its own: a document that declares nothing required in its +responses — as real ones do — sends every writable attribute to plain +`Optional`. + +OpenAPI's `default` is the document stating what the server substitutes for an +omitted value, which is that same fact in different words. It is read from the +**request** side only: a default on a response schema says nothing about what +happens when a request omits the field. + +That third route carries a known risk, accepted deliberately. A `default` on a +`$ref`'d property is written onto a schema every other use of that type shares, +so one declaration can move attributes that were never meant to move together. A +correction is the remedy where it is wrong. The alternative was leaving thousands +of attributes on plain `Optional`, which gives the practitioner a perpetual diff +on every one of them the server fills — a defect in every plan, against a risk +in some. ## Calling the workflows diff --git a/internal/intermediate_representation/attributes.go b/internal/intermediate_representation/attributes.go index 6050455..774497e 100644 --- a/internal/intermediate_representation/attributes.go +++ b/internal/intermediate_representation/attributes.go @@ -20,6 +20,10 @@ type flat struct { writeOnly bool deprecated bool uniqueItems bool + // hasDefault records that the document states a default for the + // property. What the default is never reaches a generated schema; that + // the API has one does. + hasDefault bool // The declared constraints, nil when the document states none. They // become plan-time validators. pattern string @@ -94,6 +98,9 @@ func flatten(schema *specmodel.Schema) flat { if schema.UniqueItems { flattened.uniqueItems = true } + if schema.Default != nil { + flattened.hasDefault = true + } // The declared facts about the value, folded first-wins like the // description: a branch that states one is more specific than a // branch that states nothing. @@ -475,18 +482,23 @@ func buildAttribute(wire string, attributeSite site) (Attribute, attributeEdges) attribute.ComputedOptionalRequired = Computed case attributeSite.requiredCreate: attribute.ComputedOptionalRequired = Required - case serverFills || attributeSite.requiredRead: + case serverFills || attributeSite.requiredRead || flatCreate.hasDefault: // Writable, and the response carries a value whether or not the // request supplied one: the practitioner may set it and Terraform - // must accept the server'schema choice when they dependsOnEdge not. + // must accept the server's choice when they do not. + // + // Three routes to the same fact, of decreasing authority. + // x-tfpfgen-server-default is the audit's own measurement, taken by + // omitting the attribute and reading what came back. The response + // schema's `required` list is the document asserting it. A declared + // default is the document stating what the server substitutes for an + // omitted value, which is the same claim in different words. // - // requiredRead alone is too weak to find these. It reads the response - // schema's `required` list, and an API that declares none — as real - // documents routinely do throughout — sends every writable - // optional field to plain Optional below, which is a perpetual diff - // for any field the server fills. x-tfpfgen-server-default is the - // audit'schema measurement of the same fact, and it does not depend on the - // document being diligent. + // None is redundant. requiredRead alone is too weak: an API that + // declares nothing required in its responses — as real documents + // routinely do — sends every writable optional field to plain + // Optional below, which is a perpetual diff for any field the server + // fills. attribute.ComputedOptionalRequired = ComputedOptional default: // Writable, and the server leaves it absent when the request omits it. diff --git a/internal/intermediate_representation/constraints_test.go b/internal/intermediate_representation/constraints_test.go index e835ab6..bdb1e7e 100644 --- a/internal/intermediate_representation/constraints_test.go +++ b/internal/intermediate_representation/constraints_test.go @@ -171,6 +171,82 @@ components: } } +// TestUnit_Attribute_ADeclaredDefaultFillsTheResponse proves a documented +// default sends a writable attribute to Optional + Computed: the API says it +// substitutes a value when the request omits one, so the response carries a +// value either way and plain Optional would be a perpetual diff. +func TestUnit_Attribute_ADeclaredDefaultFillsTheResponse(t *testing.T) { + const spec = `openapi: 3.0.3 +info: {title: T, version: "1"} +paths: + /jobs: + post: + requestBody: + content: + application/json: + schema: {$ref: '#/components/schemas/Job'} + responses: + "201": + content: + application/json: + schema: {$ref: '#/components/schemas/JobRead'} + /jobs/{jobId}: + get: + responses: + "200": + content: + application/json: + schema: {$ref: '#/components/schemas/JobRead'} + patch: + requestBody: + content: + application/json: + schema: {$ref: '#/components/schemas/Job'} + responses: + "200": + content: + application/json: + schema: {$ref: '#/components/schemas/JobRead'} + delete: + responses: + "204": {description: gone} +components: + schemas: + Job: + type: object + properties: + retries: {type: integer, default: 3} + enabled: {type: boolean, default: false} + label: {type: string, default: ""} + note: {type: string} + JobRead: + type: object + properties: + retries: {type: integer} + enabled: {type: boolean} + label: {type: string} + note: {type: string} + summary: {type: string, default: "none"} +` + r := resourceByKey(t, mustDerive(t, spec, testConfig()), "job") + + // A false or an empty default is a declaration like any other: the test + // is that the document states one, not that the value is truthy. + for _, name := range []string{"retries", "enabled", "label"} { + if got := attribute(t, r.Schema, name).ComputedOptionalRequired; got != ComputedOptional { + t.Errorf("%q with a declared default = %q, want computed_optional", name, got) + } + } + if got := attribute(t, r.Schema, "note").ComputedOptionalRequired; got != Optional { + t.Errorf("%q declares no default and became %q", "note", got) + } + // A default on the response side says nothing about what happens when a + // request omits the field, and summary is not writable at all. + if got := attribute(t, r.Schema, "summary").ComputedOptionalRequired; got != Computed { + t.Errorf("a response-only property with a default = %q, want computed", got) + } +} + func assertBound(t *testing.T, name string, got *int64, want int64) { t.Helper() if got == nil {