diff --git a/CHANGELOG.md b/CHANGELOG.md index af0817d..0fac0f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ All notable changes to this module are documented here, in ### Changed +- `TransactionsService.UpdateBatch` now pre-flights the import-id identity + of a `PatchByImportID` patch against the spec's 36-character bound and + answers `*ArgumentError` naming the patch index, exactly as it already + did for `payee_name` and `memo`. Previously an over-long key reached the + wire and came back a server 400. Only the import-id identity is affected: + keys of 36 characters or fewer, and every `PatchByID` patch, behave as + before — the spec declares no bound on transaction ids, so none is + enforced. - Re-vendored `openapi.yaml` from the live YNAB spec. Upstream amended one description — `SaveTransactionWithOptionalFields.subtransactions`, the shared save-transaction schema — without bumping the spec version, which diff --git a/contract_wire_test.go b/contract_wire_test.go index 06e1ffe..e90a10b 100644 --- a/contract_wire_test.go +++ b/contract_wire_test.go @@ -30,6 +30,12 @@ import ( // transaction, subtransaction and scheduled-transaction payloads bound // payee_name and memo identically, and ScheduledTransactionSpec.validate // reuses the transaction constants rather than declaring its own. +// +// A row here claims enforcement but does not prove it — the table pins the +// spec side only. Each row's teeth are a behavior test rejecting at the +// bound (mutation-verified: moving a row to the waiver table with a bogus +// reason passes this file; deleting the enforcement code does not pass the +// behavior test). A new row needs its rejection test in the same commit. var wireBounds = map[[2]string]int{ {"NewTransaction", "import_id"}: ynab.ImportIDMax, {"NewTransaction", "payee_name"}: ynab.TransactionPayeeNameMax, @@ -38,6 +44,7 @@ var wireBounds = map[[2]string]int{ {"ExistingTransaction", "memo"}: ynab.MemoMax, {"SaveTransactionWithOptionalFields", "payee_name"}: ynab.TransactionPayeeNameMax, {"SaveTransactionWithOptionalFields", "memo"}: ynab.MemoMax, + {"SaveTransactionWithIdOrImportId", "import_id"}: ynab.ImportIDMax, {"SaveTransactionWithIdOrImportId", "payee_name"}: ynab.TransactionPayeeNameMax, {"SaveTransactionWithIdOrImportId", "memo"}: ynab.MemoMax, {"SaveSubTransaction", "payee_name"}: ynab.TransactionPayeeNameMax, @@ -53,14 +60,10 @@ var wireBounds = map[[2]string]int{ // with the reason. A bound belongs here rather than in wireBounds only when // leaving it unchecked is a decision someone made; the completeness // assertion accepts either table, so the spec can never declare a bound -// that goes entirely unremarked. -var wireBoundsUnenforced = map[[2]string]string{ - {"SaveTransactionWithIdOrImportId", "import_id"}: "PatchByImportID's key reaches the wire " + - "unchecked: TransactionPatch embeds TransactionUpdate, which has no ImportID field, so " + - "UpdateBatch validates everything except this. A >36-character key comes back a server " + - "400 instead of an *ArgumentError. Tracked as a follow-up; enforcing it is a behavior " + - "change and belongs in its own commit.", -} +// that goes entirely unremarked. Currently empty: the last waiver — +// SaveTransactionWithIdOrImportId.import_id, PatchByImportID's identity — +// was lifted when TransactionPatch.validate started bounding the key. +var wireBoundsUnenforced = map[[2]string]string{} // wireEnums maps every enum the spec declares on a NAMED schema to the Go // enum type that mirrors it. The Go members come from the same AST scan diff --git a/transactions.go b/transactions.go index 00f9ffa..57b3e25 100644 --- a/transactions.go +++ b/transactions.go @@ -536,15 +536,28 @@ func PatchByID(id string, update TransactionUpdate) TransactionPatch { } // PatchByImportID addresses a batch update by import id (lookup only — -// changing an import id is not allowed by the API). Live caveat: the -// server resolves the lookup only for transactions that entered through -// the import pipeline (linked accounts); an API-created transaction -// carrying the same import_id answers 400 "transaction does not exist" -// (probed live 2026-07-20 — see API_NOTES.md). +// changing an import id is not allowed by the API). The key is bounded at +// 36 characters, the same spec bound Create enforces on +// [TransactionSpec.ImportID]. Live caveat: the server resolves the lookup +// only for transactions that entered through the import pipeline (linked +// accounts); an API-created transaction carrying the same import_id +// answers 400 "transaction does not exist" (probed live 2026-07-20 — see +// API_NOTES.md). func PatchByImportID(importID string, update TransactionUpdate) TransactionPatch { return TransactionPatch{importID: importID, TransactionUpdate: update} } +// validate applies the embedded update's bounds plus the one the identity +// carries: the spec bounds import_id at 36 characters on batch patches +// exactly as it does at creation. The id identity is deliberately not +// bounded — the spec declares no maxLength on it. +func (p TransactionPatch) validate(op string) error { + if err := checkRuneMax(op, "import_id", p.importID, importIDMax); err != nil { + return err + } + return p.TransactionUpdate.validate(op) +} + // MarshalJSON emits the update fields plus exactly one identity key. func (p TransactionPatch) MarshalJSON() ([]byte, error) { raw, err := json.Marshal(p.TransactionUpdate) diff --git a/transactions_test.go b/transactions_test.go index dca5e77..7669c2d 100644 --- a/transactions_test.go +++ b/transactions_test.go @@ -7,6 +7,7 @@ package ynab_test import ( "bytes" "encoding/json" + "io" "net/http" "net/http/httptest" "net/url" @@ -233,3 +234,53 @@ func TestUpdateBatchAnnotatesPatchIndex(t *testing.T) { require.ErrorAs(t, err, &argErr) require.Contains(t, argErr.Reason, "(patch 1)", "the failing element must be named, like CreateBatch's (spec N)") } + +func TestUpdateBatchBoundsImportIDIdentity(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, _ *http.Request) { + t.Error("no request must be sent on a pre-flight failure") + })) + t.Cleanup(srv.Close) + client := ynab.New("t", ynab.WithBaseURL(srv.URL), ynab.WithRetryDisabled()) + + // 37 code points: one past the spec's 36. The bound counts runes, not + // bytes — ééé… would be 74 bytes at 37 runes and must fail identically. + _, err := client.Plan("p-1").Transactions.UpdateBatch(t.Context(), []ynab.TransactionPatch{ + ynab.PatchByID("tr1", ynab.TransactionUpdate{Memo: ynab.Set("ok")}), + ynab.PatchByImportID(strings.Repeat("é", 37), ynab.TransactionUpdate{}), + }) + var argErr *ynab.ArgumentError + require.ErrorAs(t, err, &argErr) + require.Equal(t, "import_id", argErr.Field) + require.Contains(t, argErr.Reason, "(patch 1)") +} + +func TestUpdateBatchIDIdentityIsUnbounded(t *testing.T) { + t.Parallel() + + // The spec declares maxLength on import_id only; a long transaction id + // must reach the wire. The fake captures the body, because "unbounded" + // means sent intact — a request merely going out would also pass if a + // refactor silently dropped or truncated the identity. + bodyCh := make(chan []byte, 1) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + bodyCh <- b + _, _ = w.Write([]byte(`{"data":{"transactions":[],"transaction_ids":[],` + + `"duplicate_import_ids":[],"server_knowledge":1}}`)) + })) + t.Cleanup(srv.Close) + client := ynab.New("t", ynab.WithBaseURL(srv.URL), ynab.WithRetryDisabled()) + + _, err := client.Plan("p-1").Transactions.UpdateBatch(t.Context(), []ynab.TransactionPatch{ + ynab.PatchByID(strings.Repeat("x", 100), ynab.TransactionUpdate{}), + // And the boundary itself: exactly 36 runes of import_id pass. + ynab.PatchByImportID(strings.Repeat("é", 36), ynab.TransactionUpdate{}), + }) + require.NoError(t, err) + + body := string(<-bodyCh) + require.Contains(t, body, strings.Repeat("x", 100), "the unbounded id must reach the wire intact") + require.Contains(t, body, strings.Repeat("é", 36), "the boundary import_id must reach the wire intact") +}