Skip to content

fix: map required create-path parameters to Required attributes - #10

Merged
hanneshayashi merged 3 commits into
mainfrom
fix/resource-path-param-required
Sep 30, 2026
Merged

hanneshayashi merged 3 commits into
mainfrom
fix/resource-path-param-required

Conversation

@hanneshayashi

Copy link
Copy Markdown
Collaborator

Fixes #7.

The bug

generateResourceSchema hardcoded every read-operation parameter to ComputedOptional, so a path parameter could never become a Required attribute on a resource. datasource_mapper.go has always derived this from the OAS required flag; the resource mapper never got the same treatment. A resource addressed purely by URL therefore generated its own identity as a non-input, and providers had to overwrite the attribute in Schema() by hand.

Why required is not enough on its own

On /parents/{parent_id}/children/{child_id} both parameters are required: true — the OAS spec mandates it for path parameters, so the flag carries no information — yet they need opposite treatment: parent_id is supplied by the practitioner, child_id is assigned by the API.

Membership in the create operation's path separates them. explorer.Resource grows CreateCommonParameters and a CreateOpParameters() helper beside the existing read-path pair (CommonParameters was extracted from the read path only). Both halves are load-bearing: the scaleway fixture declares zone on the POST operation, while the kubernetes one declares namespace on the path item.

Create-operation parameters are read only to classify read parameters. They are not mapped to attributes, so the four documented resource schema sources are unchanged.

Why the classification alone would not survive

ResourceAttributes.Merge is name-keyed and target-wins, and the per-type Merge methods only ever filled an empty description — they never touched ComputedOptionalRequired. Read parameters are merged last, so a response property of the same name, forced to Computed by OverrideComputability, was already in the target slice and won. This is asset.id in terraform-provider-doit, which generates as computed rather than computed_optional.

The new mergeComputability in types.go lets Required override it. This is not a general policy change: both response bodies are built with OverrideComputability: schema.Computed, so a parameter is the only thing that can produce Required at that point in the merge. The clause is mirrored into the DataSource halves to keep the two provably identical modulo rename, where it is a no-op today because parameters are already the merge target there.

GlobalSchemaOpts.OverrideComputability deliberately stays ComputedOptional for the parameter schema — it propagates into nested properties of an object-typed parameter, and those are not themselves addressed by the URL.

Golden files

scaleway (zone, in both resources) and kubernetes (namespace) change. petstore3 and github reproduce byte-identically, which pins that the heuristic does not over-promote: pet.id comes from a read path the create operation does not share, and repository.owner likewise.

That byte-identity also means the previous corpus gave the new behaviour no coverage — no spec in it had a resource whose create and read paths differ in their path parameters. The edgecase spec grows a nested_path resource for it, with both parameters echoed in the response body so the merge collision is covered too: parent_id → required, child_id → computed.

Verified against terraform-provider-doit

Regenerated in a throwaway clone with a replace directive. Six attributes move and nothing else does:

Resource Attribute Before After
sharing resource_type, resource_id computed_optional required
label_assignments id computed_optional required
support_request_tags ticket_id computed_optional required
asset id computed required
customer customer_id computed_optional required

asset.id is the merge-precedence case. customer.customer_id is a known limitation rather than a win: it is resolved from the bearer token, and no OAS signal distinguishes it from asset.id, so that provider will keep overriding it by hand — note its existing override clears Optional and will need to clear Required instead.

user.id (create is POST /iam/v1/users/invite, no path parameter), datahub_dataset.name, cloudconnect_aws_account.account_id and the nine flat resources are untouched. Data source output is byte-identical.

Tests

  • explorer: TestCreateOpParameters_Resource, plus a config_explorer case asserting the read and create path items are extracted separately.
  • resource_mapper: seven cases covering promotion, absence from the create path, path-item-level create parameters, query parameters never being promoted, a parameter not marked required, the response-collision override, the create body keeping its own computability, and an object-typed parameter keeping its nested properties computed_optional.
  • attrmapper: a "required merge attribute - promote computability" case in each of the 24 Merge tests, plus one in each collection-dispatch test.

With mergeComputability neutered to a pass-through, every new case fails — they are not vacuous.

make test, golangci-lint run (0 issues), gofmt and go vet are all clean.

Docs

DESIGN.md gains the new rule under "Resources - Required, Computed or Optional" and a note on merge item 4.

🤖 Generated with Claude Code

Fixes #7.

`generateResourceSchema` hardcoded every read-operation parameter to
`ComputedOptional`, so a path parameter could never become a `Required`
attribute on a resource. The data source mapper has always derived this from
the OAS `required` flag; the resource mapper never got the same treatment.
A resource addressed purely by URL therefore generated its own identity as a
non-input, and providers had to overwrite the attribute in `Schema()` by hand.

`required` alone cannot make the decision. On
`/parents/{parent_id}/children/{child_id}` both parameters are `required: true`
-- the OAS spec mandates it for path parameters -- but they need opposite
treatment: `parent_id` is supplied by the practitioner, `child_id` is assigned
by the API. Membership in the create operation's path separates them, so
`explorer.Resource` grows `CreateCommonParameters` and a `CreateOpParameters()`
helper beside the existing read-path pair. Both halves are load-bearing: the
scaleway fixture declares `zone` on the POST operation, while the kubernetes
one declares `namespace` on the path item.

Fixing the classification is necessary but not sufficient, because merging
would discard it. `ResourceAttributes.Merge` is name-keyed and target-wins, and
the per-type `Merge` methods only ever filled an empty description -- they never
touched `ComputedOptionalRequired`. Read parameters are merged last, so a
response property of the same name, forced to `Computed` by
`OverrideComputability`, was already in the target slice and won. The new
`mergeComputability` in `types.go` lets `Required` override it. That is not a
general policy change: both response bodies are built with
`OverrideComputability: schema.Computed`, so a parameter is the only thing that
can produce `Required` at that point in the merge. The clause is mirrored into
the DataSource halves to keep the two provably identical modulo rename, where
it is a no-op today because parameters are already the merge target there.

`GlobalSchemaOpts.OverrideComputability` stays `ComputedOptional` for the
parameter schema. It propagates into nested properties of an object-typed
parameter, and those are not themselves addressed by the URL.

Of the five golden suites, scaleway (`zone`, twice) and kubernetes
(`namespace`) change; petstore3 and github reproduce byte-identically, which
also pins that the heuristic does not over-promote -- `pet.id` comes from a
read path the create operation does not share, and `repository.owner` likewise.
That byte-identity means the previous corpus gave the new behaviour no coverage
at all: no spec in it had a resource whose create and read paths differ in
their path parameters. The edgecase spec grows a `nested_path` resource for it,
with both parameters echoed in the response body so the merge collision is
covered too -- `parent_id` becomes `required`, `child_id` stays `computed`.

Verified end to end against terraform-provider-doit: six attributes move and
nothing else does. `sharing.resource_type`/`resource_id`,
`label_assignments.id` and `support_request_tags.ticket_id` become `required`;
`asset.id` goes from `computed` to `required`, which is the merge-precedence
case; and `customer.customer_id` becomes `required`. That last one is a known
limitation rather than a win -- it is resolved from the bearer token, and no OAS
signal distinguishes it from `asset.id`, so that provider keeps overriding it by
hand. `user.id`, `datahub_dataset.name`, `cloudconnect_aws_account.account_id`
and the nine flat resources are untouched, and the data source output is
byte-identical.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Parameter merging incorrectly treats same-name parameters in different OpenAPI locations as overrides.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
What changed in this PR

Updates resource schema generation so required create-path parameters become required Terraform attributes while preserving merge precedence.

Changes:

  • Tracks create-operation path parameters separately from read parameters.
  • Promotes parameter-derived Required computability during attribute merges.
  • Adds comprehensive unit tests, fixtures, golden outputs, and design documentation.
File Description
DESIGN.md Documents path-parameter classification and merge behavior.
internal/​explorer/​explorer.go Adds create-path common parameters to resources.
internal/​explorer/​explorer_utils.go Adds merged create-operation parameter access.
internal/​explorer/​explorer_utils_test.go Tests create parameter merging.
internal/​explorer/​config_explorer.go Extracts create path-item parameters.
internal/​explorer/​config_explorer_test.go Tests separate create/read extraction.
internal/​mapper/​resource_mapper.go Classifies shared create/read path parameters as required.
internal/​mapper/​resource_mapper_test.go Tests resource parameter classification and collisions.
internal/​mapper/​attrmapper/​types.go Adds shared computability merge logic.
internal/​mapper/​attrmapper/​string.go Applies required promotion to string merges.
internal/​mapper/​attrmapper/​string_test.go Tests string promotion.
internal/​mapper/​attrmapper/​bool.go Applies required promotion to boolean merges.
internal/​mapper/​attrmapper/​bool_test.go Tests boolean promotion.
internal/​mapper/​attrmapper/​number.go Applies required promotion to number merges.
internal/​mapper/​attrmapper/​number_test.go Tests number promotion.
internal/​mapper/​attrmapper/​int64.go Applies required promotion to integer merges.
internal/​mapper/​attrmapper/​int64_test.go Tests integer promotion.
internal/​mapper/​attrmapper/​float64.go Applies required promotion to float merges.
internal/​mapper/​attrmapper/​float64_test.go Tests float promotion.
internal/​mapper/​attrmapper/​list.go Applies required promotion to list merges.
internal/​mapper/​attrmapper/​list_test.go Tests list promotion.
internal/​mapper/​attrmapper/​list_nested.go Applies promotion to nested-list merges.
internal/​mapper/​attrmapper/​list_nested_test.go Tests nested-list promotion.
internal/​mapper/​attrmapper/​map.go Applies required promotion to map merges.
internal/​mapper/​attrmapper/​map_test.go Tests map promotion.
internal/​mapper/​attrmapper/​map_nested.go Applies promotion to nested-map merges.
internal/​mapper/​attrmapper/​map_nested_test.go Tests nested-map promotion.
internal/​mapper/​attrmapper/​set.go Applies required promotion to set merges.
internal/​mapper/​attrmapper/​set_test.go Tests set promotion.
internal/​mapper/​attrmapper/​set_nested.go Applies promotion to nested-set merges.
internal/​mapper/​attrmapper/​set_nested_test.go Tests nested-set promotion.
internal/​mapper/​attrmapper/​single_nested.go Applies promotion to nested-object merges.
internal/​mapper/​attrmapper/​single_nested_test.go Tests nested-object promotion.
internal/​mapper/​attrmapper/​resource_attributes_test.go Tests resource collection-level promotion.
internal/​mapper/​attrmapper/​data_source_attributes_test.go Tests data-source collection-level promotion.
internal/​cmd/​testdata/​scaleway/​provider_code_spec.json Updates expected required zone attributes.
internal/​cmd/​testdata/​kubernetes/​provider_code_spec.json Updates expected namespace classification.
internal/​cmd/​testdata/​edgecase/​openapi_spec.yml Adds nested-path collision fixture.
internal/​cmd/​testdata/​edgecase/​generator_config.yml Configures the nested-path resource.
internal/​cmd/​testdata/​edgecase/​provider_code_spec.json Records expected nested-path output.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/explorer/explorer_utils.go
Comment thread internal/mapper/resource_mapper_test.go Outdated
- Match operation parameters against path item parameters on (name, in)
  rather than name alone, as the OpenAPI specification identifies a
  parameter by both. Covered by a new case in each merge test.
- Rename the resource mapper case that asserts a required create path
  param promotes an optional create body property; its old name and
  comment claimed the opposite.
- Document in DESIGN.md that a parameter-derived required also overrides
  a non-required create request body property.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Same-named path and query parameters can now generate duplicate Terraform attributes.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread internal/explorer/explorer_utils.go
Parameters are identified by (name, in), attributes only by name. A path
and a query parameter sharing a name, or an alias mapping one parameter
onto another's name, previously produced a resource attribute mixing the
two (e.g. the path param's Required with the query param's description)
or a data source with a duplicated attribute that tfplugingen-framework
rejects for the whole IR file. Neither parameter can win without silently
discarding the other, so the resource or data source is now skipped with
a warning naming both parameters.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hanneshayashi
hanneshayashi merged commit 01f694b into main Sep 30, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Resource mapper never maps path parameters to Required attributes

2 participants