From dedf1355677f3d72e1a132af92930f9e6f9b19d9 Mon Sep 17 00:00:00 2001 From: ShocOne <62835948+ShocOne@users.noreply.github.com> Date: Sat, 15 Aug 2026 19:06:59 +0100 Subject: [PATCH] fix: a list result names the object by the key the resource is addressed by MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A collection GET with no id in its path is a list resource wherever a resource of the same kind exists. Classification says so and derives 34 of them from ThousandEyes; emission served 11, and narrowed the rule for a difference in wording. A list result is an identity and an identity is the resource's id. The resource takes its id from the item path key where the object declares no property of that name — ensureID has always done this — but the list element did not, so an API spelling its key testId rather than id published no identity and lost the entity outright. Deriving the element's id the same way exposed a second refusal behind the first. An identity is the parents that scope an object plus its id, and the item path key is the id; counting it as addressing too required the same value under two names, of every import and of every list result, wherever the document also declared that key as a property. List resources go 73 to 98: ThousandEyes 11 to 30, GitHub 23 to 29, Jamf Pro unchanged. What still refuses names the key it looked for and the fields the element does carry, so the eleven that need the field named as data can be found without reading the toolkit. Co-Authored-By: Claude Opus 5 (1M context) --- docs/emittance_tracker.md | 22 +-- internal/emit/render_identity.go | 30 ++++- internal/emit/render_identity_test.go | 82 +++++++++++ internal/emit/render_listresource.go | 31 ++++- internal/emit/render_listresource_test.go | 88 ++++++++++++ .../intermediate_representation/derive.go | 16 ++- .../derive_list_resource_test.go | 127 ++++++++++++++++++ .../derive_test.go | 45 +------ .../intermediate_representation_test.go | 12 ++ 9 files changed, 400 insertions(+), 53 deletions(-) create mode 100644 internal/emit/render_identity_test.go create mode 100644 internal/intermediate_representation/derive_list_resource_test.go diff --git a/docs/emittance_tracker.md b/docs/emittance_tracker.md index 83edf2c..80d883d 100644 --- a/docs/emittance_tracker.md +++ b/docs/emittance_tracker.md @@ -26,30 +26,36 @@ were measured against, so a stale row is visible as a stale row. ## Measurement of 2026-08-15 -Toolkit at `8c68058`. Every tree regenerates byte-identical under `tfpfgen -provider verify`. +Toolkit at `list-identity-from-item-key`. Every tree regenerates byte-identical +under `tfpfgen provider verify`. | Document | Provider tree files | Resources | Data sources | List resources | Actions | Builds | |---|---|---|---|---|---|---| | Jamf Pro | 3915 | 76 | 211 | 39 | 101 | yes | -| GitHub | 4406 | 59 | 323 | 23 | 77 | yes | -| ThousandEyes | 1798 | 35 | 95 | 11 | 51 | yes | -| Total | 10119 | 170 | 629 | 73 | 229 | | +| GitHub | 4442 | 59 | 323 | 29 | 77 | yes | +| ThousandEyes | 1912 | 35 | 95 | 30 | 51 | yes | +| Total | 10269 | 170 | 629 | 98 | 229 | | Refusals, by the stage that refused: | Document | Total | Derivation | Binding | Emission | |---|---|---|---|---| | Jamf Pro | 255 | 102 | 136 | 17 | -| GitHub | 845 | 330 | 494 | 21 | -| ThousandEyes | 361 | 87 | 251 | 23 | -| Total | 1461 | 519 | 881 | 61 | +| GitHub | 839 | 330 | 494 | 15 | +| ThousandEyes | 342 | 87 | 251 | 4 | +| Total | 1436 | 519 | 881 | 36 | Binding refuses most of what is refused, and that is the expected shape: it is the only stage that resolves a drafted mapping against the SDK that was actually generated, so it is where a document's ambition meets what the backend could carry. +Eleven of the emission refusals are one shape: a list element whose key the +document spells its own way, where no rule derives that spelling from the path +— `/roles/{id}` beside an element carrying `roleId`, `/users/{id}` beside +`uid`. Each names its candidates in its reason. They need the field named as +data before they can publish an identity. + ## The documents Each is pinned by SHA-256 in its own provider repo's `spec/upstream.lock.json`. diff --git a/internal/emit/render_identity.go b/internal/emit/render_identity.go index aa4cd22..b600077 100644 --- a/internal/emit/render_identity.go +++ b/internal/emit/render_identity.go @@ -47,7 +47,7 @@ func resourceIdentity(r *ir.Resource) []identityAttribute { if r.Schema == nil { return nil } - addressing := addressingNames(r.Operations.Read, r.Operations.Create, r.Operations.Delete) + addressing := identityAddressing(r.Operations.Read, r.Operations.Create, r.Operations.Delete) var out []identityAttribute var carriesID bool @@ -84,6 +84,34 @@ func resourceIdentity(r *ir.Resource) []identityAttribute { return out } +// identityAddressing is the path parameters that scope an object, which is +// every one but the parameter naming the object itself. +// +// That last parameter is the id, and the id is added to the identity by name. +// Counting it as addressing as well puts one value in the identity twice +// wherever the document also declares it as a property — /alerts/rules/{ruleId} +// beside a ruleId field — and the duplicate is required for import and +// required of every list result, which only the resource can supply. +// +// A path not ending in a parameter addresses a collection, so every parameter +// on it is a parent. +func identityAddressing(operations ...*ir.Operation) map[string]bool { + names := map[string]bool{} + for _, operation := range operations { + if operation == nil { + continue + } + parameters := operation.PathParameters + if len(parameters) > 0 && strings.HasSuffix(operation.PathTemplate, "}") { + parameters = parameters[:len(parameters)-1] + } + for _, parameter := range parameters { + names[ir.TerraformName(parameter.Name)] = true + } + } + return names +} + // identitySchemaDecls renders the identity schema's attribute declarations, // ready to sit inside a map[string]identityschema.Attribute literal. // diff --git a/internal/emit/render_identity_test.go b/internal/emit/render_identity_test.go new file mode 100644 index 0000000..27cab94 --- /dev/null +++ b/internal/emit/render_identity_test.go @@ -0,0 +1,82 @@ +package emit + +import ( + "reflect" + "testing" + + ir "github.com/deploymenttheory/terraform-plugin-framework-codegen/internal/intermediate_representation" +) + +// names of a resource's identity, in order, for comparison. +func identityNames(identity []identityAttribute) []string { + out := make([]string, 0, len(identity)) + for _, a := range identity { + out = append(out, a.Name) + } + return out +} + +// An identity names one object: the parents that scope it, then its id. The +// parameter naming the object itself is the id and is not also addressing, +// which matters wherever the document declares that key as a property too — +// the identity would otherwise require the same value under two names, of +// every import and of every list result. +func TestUnit_ResourceIdentity_TheItemKeyIsTheIDAndNotAlsoAddressing(t *testing.T) { + for _, tc := range []struct { + name string + read *ir.Operation + tree *ir.AttributeTree + want []string + }{ + { + name: "the body declares the item key as a property", + read: &ir.Operation{Kind: ir.OperationRead, Method: "GET", + PathTemplate: "/alerts/rules/{ruleId}", + PathParameters: []ir.Parameter{{Name: "ruleId", Type: ir.TypeString}}}, + tree: &ir.AttributeTree{Attributes: []ir.Attribute{ + {Name: "id", WireName: "ruleId", Kind: ir.TypeString, ComputedOptionalRequired: ir.Computed}, + {Name: "rule_id", WireName: "ruleId", Kind: ir.TypeString, ComputedOptionalRequired: ir.Computed}, + }}, + want: []string{"id"}, + }, + { + name: "a parent scopes the object", + read: &ir.Operation{Kind: ir.OperationRead, Method: "GET", + PathTemplate: "/repos/{owner}/hooks/{hookId}", + PathParameters: []ir.Parameter{ + {Name: "owner", Type: ir.TypeString}, + {Name: "hookId", Type: ir.TypeString}, + }}, + tree: &ir.AttributeTree{Attributes: []ir.Attribute{ + {Name: "owner", WireName: "owner", Kind: ir.TypeString, ComputedOptionalRequired: ir.Required}, + {Name: "id", WireName: "hookId", Kind: ir.TypeString, ComputedOptionalRequired: ir.Computed}, + {Name: "hook_id", WireName: "hookId", Kind: ir.TypeString, ComputedOptionalRequired: ir.Computed}, + }}, + want: []string{"owner", "id"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + got := identityNames(resourceIdentity(&ir.Resource{ + Schema: tc.tree, + Operations: ir.Operations{Read: tc.read}, + })) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("identity = %v, want %v", got, tc.want) + } + }) + } +} + +// A collection path names no object, so every parameter on it is a parent +// and none of them is dropped as an item key. +func TestUnit_ResourceIdentity_ACollectionPathKeepsEveryParameter(t *testing.T) { + got := identityAddressing(&ir.Operation{ + Kind: ir.OperationCreate, Method: "POST", + PathTemplate: "/orgs/{org}/teams", + PathParameters: []ir.Parameter{ + {Name: "org", Type: ir.TypeString}, + }}) + if !got["org"] { + t.Errorf("a collection path's parameter is addressing, got %v", got) + } +} diff --git a/internal/emit/render_listresource.go b/internal/emit/render_listresource.go index 653c123..f239b21 100644 --- a/internal/emit/render_listresource.go +++ b/internal/emit/render_listresource.go @@ -3,6 +3,7 @@ package emit import ( "fmt" "path" + "sort" "strconv" "strings" @@ -236,7 +237,9 @@ func (e *serviceRenderer) listResource(lr *ir.ListResource, lb *sdkbind.ListReso func listResultLines(nodes []node, identity []identityAttribute, config []node) (string, error) { idNode, ok := findIdentityNode(nodes) if !ok { - return "", unrenderable("the element carries no scalar id attribute to publish as the list identity") + return "", unrenderable( + "the element publishes no identity: it carries no readable scalar %q%s", + idAttributeName, identityCandidates(nodes)) } configured := map[string]bool{} @@ -277,6 +280,32 @@ func listResultLines(nodes []node, identity []identityAttribute, config []node) return b.String(), nil } +// identityCandidates names the readable scalars whose spelling suggests they +// key the object, so a refusal says what the element does carry rather than +// only what it lacks. An operator reads it to decide which one to name in a +// correction; without it the only way to find them is to open the document. +func identityCandidates(nodes []node) string { + var found []string + for _, n := range nodes { + if n.attr.Nested != nil || n.fb == nil || n.fb.Access.Get == "" { + continue + } + switch n.attr.Kind { + case ir.TypeString, ir.TypeInt64, ir.TypeFloat64: + default: + continue + } + if strings.HasSuffix(n.attr.Name, "id") { + found = append(found, n.attr.WireName) + } + } + if len(found) == 0 { + return ", and no readable scalar it carries is spelled like a key" + } + sort.Strings(found) + return fmt.Sprintf(", though it carries %s", strings.Join(found, ", ")) +} + // findStringNode finds a plain string attribute by name. func findStringNode(nodes []node, name string) (node, bool) { for _, n := range nodes { diff --git a/internal/emit/render_listresource_test.go b/internal/emit/render_listresource_test.go index 6529d08..4412efc 100644 --- a/internal/emit/render_listresource_test.go +++ b/internal/emit/render_listresource_test.go @@ -156,3 +156,91 @@ func TestUnit_ListResource_WithoutAddressingDeclaresNoConfiguration(t *testing.T t.Errorf("an unparameterised collection path is mocked by exact URL:\n%s", test) } } + +// An API that spells its key after the thing it identifies gives the element +// an id whose wire name is the item path key, which is what derivation now +// puts there. Emission has to publish that as the identity: refusing it lost +// every entity whose document simply worded its key differently. +func TestUnit_ListResource_PublishesAnIdentityKeyedTheAPIsWay(t *testing.T) { + m, b := fictionalModel(), fictionalBindings() + + lr := &m.ListResources[0] + if lr.Names.Key != "http_server" { + t.Fatalf("the fictional list resource moved: %q", lr.Names.Key) + } + // The element carries the key as the API words it, not as "id". + for i := range lr.Schema.Attributes { + if lr.Schema.Attributes[i].Name == "id" { + lr.Schema.Attributes[i].WireName = "httpServerId" + } + } + lb := b.ListResources["http_server"] + fields := make([]sdkbind.FieldBinding, 0, len(lb.Fields)) + for _, f := range lb.Fields { + if f.Attr == "id" { + f.Wire = "httpServerId" + f.Access = readOnly(kAccess("HttpServerId", "*string", "FromPtrString", "", "")) + } + fields = append(fields, f) + } + lb.Fields = fields + + out, err := RenderServices(fictionalProviderCore(), m, b) + if err != nil { + t.Fatalf("an element keyed the API's way must still render: %v", err) + } + list := string(fileByPath(t, out, "internal/services/list-resources/servers/v7/http_server/list.go").Content) + if !strings.Contains(list, "GetHttpServerId()") { + t.Errorf("the identity is not read from the element's own key:\n%s", list) + } + if !strings.Contains(list, "identityModel{ID: types.StringValue(id)}") { + t.Errorf("the element's key is not published as the identity:\n%s", list) + } +} + +// The refusal an element with no key at all still earns has to say what it +// looked for and what the element does carry, or the only way to write the +// correction is to read the toolkit. +func TestUnit_ListResource_RefusalNamesWhatTheElementCarries(t *testing.T) { + m, b := fictionalModel(), fictionalBindings() + + lr := &m.ListResources[0] + kept := make([]ir.Attribute, 0, len(lr.Schema.Attributes)) + for _, a := range lr.Schema.Attributes { + if a.Name == "id" { + a.Name, a.WireName = "server_uid", "serverUid" + } + kept = append(kept, a) + } + lr.Schema.Attributes = kept + lb := b.ListResources["http_server"] + fields := make([]sdkbind.FieldBinding, 0, len(lb.Fields)) + for _, f := range lb.Fields { + if f.Attr == "id" { + f.Attr, f.Wire = "server_uid", "serverUid" + f.Access = readOnly(kAccess("ServerUid", "*string", "FromPtrString", "", "")) + } + fields = append(fields, f) + } + lb.Fields = fields + + out, err := RenderServices(fictionalProviderCore(), m, b) + if err != nil { + t.Fatalf("one refused list resource must not fail the run: %v", err) + } + + var reason string + for _, e := range out.Excluded { + if e.Key == "http_server" { + reason = e.Reason + } + } + if reason == "" { + t.Fatalf("the refusal was not reported: %+v", out.Excluded) + } + for _, want := range []string{`no readable scalar "id"`, "serverUid"} { + if !strings.Contains(reason, want) { + t.Errorf("the refusal does not mention %q: %s", want, reason) + } + } +} diff --git a/internal/intermediate_representation/derive.go b/internal/intermediate_representation/derive.go index ef79c2f..beec370 100644 --- a/internal/intermediate_representation/derive.go +++ b/internal/intermediate_representation/derive.go @@ -458,10 +458,24 @@ func (derivation *deriver) listResource(classification specmodel.Classification, listOperation := *derivation.operation(classification.List, OperationList) addressing := addressingSchema(listOperation.PathParameters) refuseReservedRootNames(addressing) + + // A list result is an identity, and an identity is the resource's id. The + // resource takes its id from the item path key where the object declares + // no property of that name, and the element has to answer the same key by + // the same rule: /tests/{testId} keys the object on testId whether it is + // being read one at a time or streamed. + // + // Without this the element kept only the document's own spelling, and an + // API that does not happen to call its key "id" published no identity at + // all — which refused the entity outright, for a difference in wording. + tree := buildTree(nil, element, nil, false) + keyParam, keyType := itemKeyParam(classification.ItemPath, derivation.full(classification.Read)) + ensureID(tree, keyParam, keyType) + return ListResource{ Names: names, ListOperation: listOperation, - Schema: buildTree(nil, element, nil, false), + Schema: tree, AddressingSchema: addressing, ListEnvelopeKey: listEnvelopeKey(listFull), } diff --git a/internal/intermediate_representation/derive_list_resource_test.go b/internal/intermediate_representation/derive_list_resource_test.go new file mode 100644 index 0000000..b7b0dd9 --- /dev/null +++ b/internal/intermediate_representation/derive_list_resource_test.go @@ -0,0 +1,127 @@ +package intermediate_representation + +import "testing" + +const keyedSpec = `openapi: 3.0.3 +info: {title: K, version: "1"} +paths: + /gadgets: + post: + operationId: createGadget + requestBody: + content: + application/json: + schema: {$ref: '#/components/schemas/Gadget'} + responses: + "201": + content: + application/json: + schema: {$ref: '#/components/schemas/Gadget'} + get: + operationId: listGadgets + responses: + "200": + content: + application/json: + schema: + type: array + items: {$ref: '#/components/schemas/Gadget'} + /gadgets/{gadgetId}: + parameters: + - {name: gadgetId, in: path, required: true, schema: {type: string}} + get: + operationId: getGadget + responses: + "200": + content: + application/json: + schema: {$ref: '#/components/schemas/Gadget'} + delete: + operationId: deleteGadget + responses: + "204": {description: gone} +components: + schemas: + Gadget: + type: object + properties: + gadgetId: {type: string} + name: {type: string} +` + +// A list result is an identity and an identity is the resource's id, so the +// element has to answer the key the resource is addressed by. An API that +// spells that key after the thing it identifies used to publish no identity +// at all, and the entity was refused downstream for the wording. +func TestDerive_ListResourceTakesItsIDFromTheItemPathKey(t *testing.T) { + m := mustDerive(t, keyedSpec, testConfig()) + + lr := listResourceByKey(t, m, "gadget") + id := attribute(t, lr.Schema, "id") + if id.WireName != "gadgetId" { + t.Errorf("the list element's id reads %q, want the item path key %q", id.WireName, "gadgetId") + } + if id.Kind != TypeString || id.ComputedOptionalRequired != Computed { + t.Errorf("id = %+v, want a computed string", id) + } + // The resource is addressed by the same key, and terraform matches the + // two by type name: an identity that named a different field would list + // identities the resource cannot be imported by. + if got := attribute(t, resourceByKey(t, m, "gadget").Schema, "id"); got.WireName != id.WireName { + t.Errorf("resource id reads %q but the list element's reads %q", got.WireName, id.WireName) + } +} + +// An element that declares its own id keeps it: ensureID returns early, so +// the entities that published an identity before still publish the same one. +func TestDerive_ListResourceKeepsAnElementsOwnID(t *testing.T) { + m := mustDerive(t, thingSpec, testConfig()) + + id := attribute(t, listResourceByKey(t, m, "thing").Schema, "id") + if id.WireName != "id" { + t.Errorf("id reads %q, want the element's own %q", id.WireName, "id") + } +} + +// TestDerive_ListResource proves the list capability belongs to a resource +// and shares its terraform type, which is how terraform matches the two. +func TestDerive_ListResource(t *testing.T) { + m := mustDerive(t, thingSpec, testConfig()) + for _, lr := range m.ListResources { + if lr.Names.Key != "thing" { + continue + } + if lr.ListOperation.Kind != OperationList || lr.ListOperation.Method != "GET" || lr.ListOperation.SuccessCode != 200 { + t.Errorf("list op = %+v", lr.ListOperation) + } + resource := resourceByKey(t, m, "thing") + if lr.Names.TerraformType != resource.Names.TerraformType { + t.Errorf("list resource type = %q, resource type = %q; terraform matches them by name", + lr.Names.TerraformType, resource.Names.TerraformType) + } + for _, name := range []string{"name", "id"} { + if a := attribute(t, lr.Schema, name); a.ComputedOptionalRequired != Computed { + t.Errorf("%q = %+v", name, a) + } + } + return + } + t.Fatalf("no thing list resource in %+v", m.ListResources) +} + +// TestDerive_ListOnlyEntityIsADatasource proves a collection the API cannot +// address one member of yields a datasource: no resource can match it, and +// terraform refuses a provider whose list resource names no resource. +func TestDerive_ListOnlyEntityIsADatasource(t *testing.T) { + m := mustDerive(t, thingSpec, testConfig()) + for _, lr := range m.ListResources { + if lr.Names.Key == "event" { + t.Fatalf("event is enumerable but not addressable, and became a list resource") + } + } + datasourceByKey(t, m, "event") +} + +// TestUnit_AddressingSchema_TakesEveryPathParameter proves a collection +// path's parameters all become required attributes of the list block: a +// collection path carries no item key, so none of them is absorbed by an id, diff --git a/internal/intermediate_representation/derive_test.go b/internal/intermediate_representation/derive_test.go index 5914c58..5fe75d2 100644 --- a/internal/intermediate_representation/derive_test.go +++ b/internal/intermediate_representation/derive_test.go @@ -300,48 +300,9 @@ func TestDerive_ListReadEntityYieldsDatasource(t *testing.T) { } } -// TestDerive_ListResource proves the list capability belongs to a resource -// and shares its terraform type, which is how terraform matches the two. -func TestDerive_ListResource(t *testing.T) { - m := mustDerive(t, thingSpec, testConfig()) - for _, lr := range m.ListResources { - if lr.Names.Key != "thing" { - continue - } - if lr.ListOperation.Kind != OperationList || lr.ListOperation.Method != "GET" || lr.ListOperation.SuccessCode != 200 { - t.Errorf("list op = %+v", lr.ListOperation) - } - resource := resourceByKey(t, m, "thing") - if lr.Names.TerraformType != resource.Names.TerraformType { - t.Errorf("list resource type = %q, resource type = %q; terraform matches them by name", - lr.Names.TerraformType, resource.Names.TerraformType) - } - for _, name := range []string{"name", "id"} { - if a := attribute(t, lr.Schema, name); a.ComputedOptionalRequired != Computed { - t.Errorf("%q = %+v", name, a) - } - } - return - } - t.Fatalf("no thing list resource in %+v", m.ListResources) -} - -// TestDerive_ListOnlyEntityIsADatasource proves a collection the API cannot -// address one member of yields a datasource: no resource can match it, and -// terraform refuses a provider whose list resource names no resource. -func TestDerive_ListOnlyEntityIsADatasource(t *testing.T) { - m := mustDerive(t, thingSpec, testConfig()) - for _, lr := range m.ListResources { - if lr.Names.Key == "event" { - t.Fatalf("event is enumerable but not addressable, and became a list resource") - } - } - datasourceByKey(t, m, "event") -} - -// TestUnit_AddressingSchema_TakesEveryPathParameter proves a collection -// path's parameters all become required attributes of the list block: a -// collection path carries no item key, so none of them is absorbed by an id, +// keyedSpec is a full lifecycle whose objects are keyed by "gadgetId" +// rather than by "id" — the ordinary case in an API that names its keys +// after the thing they identify. // and none carries RequiresReplace because a list block has no plan. func TestUnit_AddressingSchema_TakesEveryPathParameter(t *testing.T) { if tree := addressingSchema(nil); tree != nil { diff --git a/internal/intermediate_representation/intermediate_representation_test.go b/internal/intermediate_representation/intermediate_representation_test.go index 0059a60..e583618 100644 --- a/internal/intermediate_representation/intermediate_representation_test.go +++ b/internal/intermediate_representation/intermediate_representation_test.go @@ -68,6 +68,18 @@ func resourceByKey(t *testing.T, m *Model, key string) Resource { return Resource{} } +// listResourceByKey finds a model list resource, or ends the test. +func listResourceByKey(t *testing.T, m *Model, key string) ListResource { + t.Helper() + for _, lr := range m.ListResources { + if lr.Names.Key == key { + return lr + } + } + t.Fatalf("no list resource %q in the model", key) + return ListResource{} +} + // datasourceByKey finds a model datasource, or ends the test. func datasourceByKey(t *testing.T, m *Model, key string) Datasource { t.Helper()