feat(resource): add read_search.id_attribute for envelope-wrapped APIs - #365
Open
mbrownnycnyc wants to merge 9 commits into
Open
feat(resource): add read_search.id_attribute for envelope-wrapped APIs#365mbrownnycnyc wants to merge 9 commits into
mbrownnycnyc wants to merge 9 commits into
Conversation
Some APIs wrap objects differently on create vs. in the list endpoint -
e.g. POST returns {"data":{"id":N}} (needing id_attribute="data/id")
while the list/collection endpoint returns flat items {"id":N} (needing
"id"). A single object-wide id_attribute cannot serve both, so read_search
fails to extract the id from the flat search item, the read returns empty,
and the resource layer errors with "unexpected end of JSON input".
Add an optional read_search.id_attribute that overrides the object-wide
id_attribute when locating the id within a search result item. Defaults to
the object-wide id_attribute, so existing configs are unaffected.
Repro: pfSense-pkg-RESTAPI (pfrest) wraps every response as
{code,status,...,data:<obj|array>}; create needs data/id, the list returns
flat items.
Table test in object_read_search_test.go: with read_search.id_attribute the id is read from the flat list item; without it, the object-wide "data/id" misses the flat item and the read drops the object from state.
… bodies
Some APIs read the object id from the request BODY rather than the URL/query
whenever the request carries a JSON content type. pfSense pfrest is one: it
returns MODEL_REQUIRES_ID for a bodyless PATCH/DELETE even when ?id=N is set,
because it only looks for `id` in the body. A generic restapi config cannot
satisfy this - the provider only substitutes {id} into the path.
Add an optional `body_id_attribute` (resource- and provider-level). When set,
the object's id is merged into the JSON body of UPDATE and DELETE requests
under that key, as a JSON number when it parses as an integer (pfrest's model
validators require an integer id), otherwise as a string. Empty/unset = current
behavior. Settable at the provider level so it also covers destroying objects
whose state predates the setting.
Tests: pure helper table test (empty/merge/numeric/string/invalid) plus httptest
coverage asserting the id lands in the DELETE and UPDATE bodies, and that the
default (unset) still sends no delete body.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e/delete Some APIs use positional/array-index ids that renumber when a sibling object is deleted or created earlier in the same apply (e.g. pfSense pfrest renumbers host overrides on delete). The id captured at refresh then goes stale and the write hits the wrong object. Add read_search.resolve_before_write: when true, the object's id is re-resolved from the live collection (by the unique search key) immediately before each UPDATE and DELETE. When the object is no longer present, delete is a no-op (already gone). Pair with -parallelism=1 so writes serialize and no concurrent delete shifts the index between the lookup and the request. Tests: delete re-resolves a stale id (state 5 -> live 4) and targets the live id; not-found short-circuits to a no-op delete. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…_before_write) The previous read_search.resolve_before_write was a per-resource attribute, so adding it to existing resources triggered an in-place UPDATE - which both hits the unrelated envelope-unwrap bug and (for side-effectful objects like ACME certs and HAProxy frontends) would re-issue/reload them. Move it to a provider-level resolve_id_before_write bool (mirrors body_id_attribute): enabling it does not churn existing resources, and it still re-resolves the id by read_search right before each update/delete for positional-id APIs. Tests updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ModifyPlan only flagged a force_new field for replacement when it already
existed in prior state and its value changed (the check was gated on
getNestedValue(stateData, field) returning no error). A force_new attribute
that was NEWLY ADDED - absent in state, present in the plan - was therefore
silently planned as an in-place update.
Treat add (absent in state, present in plan), remove (present in state, absent
in plan), and value change all as triggers for replacement.
Adds TestAccRestApiObject_ForceNewAddedField (fakeserver + plancheck
DestroyBeforeCreate), which fails on the old logic ("got action(s): [update]")
and passes with this change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ope (bug Mastercard#4) Some APIs (e.g. pfSense pfrest) return a {code,status,...,data:{...}} ENVELOPE from a PUT/PATCH instead of the bare object. UpdateObject previously parsed that envelope straight into apiData via updateInternalState whenever write_returns_object was set, poisoning state with the envelope keys. Terraform then reported "Provider produced inconsistent result after apply" on every in-place update, which is why all pfrest resources had to stay force_new (destroy+recreate). Now, when read_search is configured, UpdateObject re-reads the object via read_search after the write so apiData holds the clean, unwrapped object. Direct-parse is preserved for write_returns_object APIs without read_search (bare-object responses), so non-pfrest behavior is unchanged. Adds TestUpdateObject_ReadSearchUnwrapsEnvelope: a mock that returns an envelope on PUT and the bare object on the read_search GET, asserting apiData holds the unwrapped object and none of the envelope keys leak in. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…(pfrest) ImportState read objects by a direct id-path, which returns empty for pfrest collection endpoints -> data unset -> 'Invalid JSON String Value'. Configure a transient read_search (search_key=id, results_key=data, id_attribute=id) so ReadObject locates the object in its list and populates data from the live body. Cleared before State.Set so it never persists or conflicts with the resource's own configured read_search on the next plan.
…ption) Import a collection object by a unique field instead of an id-path: persist a matching read_search (results_key=data, id_attribute=id) so the post-import refresh works AND equals the managed resource's read_search; with ignore_server_additions the first plan is a clean no-op. Bare <path>/<id> still works. Placeholder id satisfies ReadObject's id!=empty guard; FindObject sets the real id.
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
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.
What
Add an optional
id_attributeto theread_searchblock ofrestapi_object, overriding theobject-wide
id_attributewhen extracting the matched record's id from a search-result item.Why
Some APIs wrap the object differently on create vs. in the list/collection endpoint:
POST /widgets->{"data": {"id": 123, "name": "foo"}}- the id is atdata/id, so you set theobject-wide
id_attribute = "data/id".GET /widgets(theread_searchsource) ->{"data": [{"id": 123, "name": "foo"}]}- the listitems are flat, with the id at
id.A single object-wide
id_attributecan't satisfy both. Withid_attribute = "data/id",read_searchcannot find the id in the flat list item, so
FindObjecterrors, the read returns an empty body, andthe resource fails on refresh with:
This affects any API that wraps all responses in an envelope (e.g.
{code,status,data:{...}}oncreate and
{code,status,data:[...]}on list).What changed
read_searchgains an optionalid_attribute. When set,FindObjectreads the matched item's idfrom that key; otherwise it falls back to the object-wide
id_attribute. No behavior change forexisting configs - fully backward compatible.
internal/provider/resource_api_object.go(schema + model + map wiring),internal/apiclient/object.go(FindObject), and a table test ininternal/apiclient/object_read_search_test.go.Example
Notes
docs/are generated via tfplugindocs; I was not able to runmake docsin myenvironment - happy to regenerate, or a maintainer can.