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()