Add support for addLedgerEntries

[vigneshwerv]

No description provided.

[snoble]

Review by Claude, running in Steven's session — these are Claude's words and judgements, not Steven's.

Recovering the per-entry-type shapes from the single-entry addLedgerEntry operations is a genuinely good idea: the information really is sitting in the queries directory, and nothing else can close the parameters: JSON gap for a batch. The EntrySpec → render split is clean, the reasoning is documented where it's non-obvious, and reading the base class's attribute names out of its own AST instead of hand-listing them is the right instinct.

Three things I'd want changed before this ships, then nits.

1. Both README batch examples are rejected by the API today

addLedgerEntries currently enforces a homogeneous batch: one entry type, one type version, one ledger, unique iks (assertHomogeneousBatch in the API). The typed-payload example mixes two entry types:

entries=[AuthCaptureV1(...), PlatformFundsAccountV1(...)]

That comes back as invalid_input_provided: "entries[1] uses entry type 'platform_funds_account' V1; all entries in a batch must use 'auth_capture' V1." The CHANGELOG line "mixed with raw AddLedgerEntryInput values" is fine — mixing forms works, mixing types doesn't — but the example should show one type per batch, and the constraint is worth stating outright since the typed models make violating it feel natural.

Second, both README examples omit headers={"X-Fragment-Experimental": "true"}, which the endpoint requires (tests/test_add_ledger_entries.py passes it, so the tests pass and a reader following the README doesn't). Keeping the header out of the client is the right call; it just has to be in the docs.

Heads-up while you're here: a batch-wide cap of 30 Ledger Lines between the entries is in flight on the API side (in addition to the existing 30-per-entry limit). Worth a sentence in the README once it lands, since a 16-member batch of 2-line entries will start failing.

2. assign_class_names mutates its input and isn't idempotent

fragment/codegen/typed_entries.py — base = f"{spec.class_name}V{version}" reads class_name, then assigns back into it. Call it twice on the same specs and you get OrderPlacedV1V1. Today that's latent because render_module is called once, but _init_additions() in the plugin reads spec.class_name after render_module mutated it, so the __init__ re-exports are correct only because of hook ordering. Two small things fix it:

• make it pure — return new specs (or a dict[(type, version), str] name map) instead of assigning to spec.class_name;
• EntrySpec.class_name currently means "unversioned pascal name" before the call and "final model name" after. Two fields (base_name, class_name) or a pure function make that meaning stable.

3. V1 in the name, no typeVersion on the wire

OrderPlacedV1 posts no typeVersion at all when the operation pins none, while OrderPlacedV2 posts 2. The docstring explains it, but a caller reading V1 will reasonably assume 1 is being sent. The API treats a missing typeVersion as 1 (entry.typeVersion ?? 1 in assertHomogeneousBatch), so emitting TYPE_VERSION: ClassVar[Optional[int]] = 1 for the unpinned case would be wire-equivalent and make the name honest. If you'd rather keep "unspecified" distinct from "explicitly 1" — a defensible position — then the class name is the thing to soften, because right now the two disagree.

Pythonic

• ast.Name(id="Sequence[Union[AddLedgerEntryInput, TypedLedgerEntry]]") (plugins/generate_typed_entries.py) is a whole expression smuggled into an identifier. It survives ast.unparse but it isn't a valid AST, so anything that validates or visits the tree (or compile()) breaks on it. ast.parse("Sequence[Union[...]]", mode="eval").body gives you the real node for free.
• module_path.write_text(...) — pass encoding="utf-8". On Windows this silently picks up cp1252 and mangles any non-ASCII parameter name or docstring. Consider parents=True on the directory too, so the hook doesn't depend on ariadne having created the package dir first.
• The package targets ^3.10, so the hand-written codegen modules can use dict[str, str], list[EntrySpec], str | None rather than Dict/List/Optional. (The generated module should keep matching ariadne's style — that inconsistency is forced.)
• Missing annotations: _unwrap_type(type_node) -> tuple wants -> tuple[str, bool] and a typed parameter; _get_object_field wants -> ValueNode | None; seen: set = set() wants set[str]. Also for f in node.fields → for field_node in ..., since field is already imported from dataclasses in this module.
• @lru_cache(maxsize=1) on a zero-argument function is doing the job of a module constant; functools.cache reads better if you want the laziness.

Tests

The end-to-end coverage is the right shape — exercising the snapshotted client is exactly what I'd want, and using a raw input for the unknown-entry-type case is a nice touch. What's missing is anything that runs without live credentials, and it happens to be the trickiest logic in the PR:

• PARAMETER_FIELDS keeping the Schema name when the Python field is escaped (type_, class_, json_) — the README promises this and nothing tests it;
• camelCase parameter → snake_case field, with the wire key unchanged;
• to_input() / serialize() shape, including typeVersion omitted when unpinned;
• assign_class_names on two versions of one type, and on the pascal-case collision path (auth_hold vs authHold).

Those are all pure functions over an EntrySpec or a model instance, so they're fast unit tests, and they'd have caught the idempotency bug above. Also test_add_ledger_entries_rejects_unknown_entry_type stores a second Schema and Ledger it never uses — a session-scoped fixture would halve the setup.

Smaller

• TypedLedgerEntry exposes posted, tags, groups, conditions but not description, which LedgerEntryInput accepts. Deliberate?
• @model_serializer means model_dump() on a typed entry returns the AddLedgerEntryInput shape, so Model.model_validate(instance.model_dump()) won't round-trip. Fine for sending, surprising for anyone who logs or caches these — maybe worth a line in the docstring.
• _extract_parameters falls back to "Any" when a variable's annotation can't be found. Silent is the wrong default for codegen; a warning would tell the user their parameter lost its type.
• _widen_entries_argument no-ops silently if the entries argument isn't found (e.g. ariadne renames it), and the typed models then don't typecheck for users. Worth a warning there too.
• poetry.toml with in-project = true is a local preference being committed for everyone — intentional?

[vigneshwerv]

Pushed bef1a9b, then 4cc6201 with the unit tests.

Idempotency (#2). resolve_class_names is pure now. It returns (name, spec) pairs and never writes back into a spec. EntrySpec.class_name became base_name and always holds the unversioned name, with a versioned_name property deriving the rest, so neither field's meaning depends on what has already run. The plugin's _init_additions calls resolve_class_names itself instead of reading names off the specs, which removes the hook-ordering dependency you spotted. I reproduced OrderPlacedV1V1 first. Putting the assignment back makes four of the new unit tests fail.

typeVersion (#3). Took your first option. An unpinned operation normalizes to 1 at extraction, so the name matches what goes on the wire, and the caveat is out of the docstring. One consequence: an unpinned operation and one pinning typeVersion: 1 now share an identity, so they collapse into one model. That follows from the resolution rule you described, and it wasn't true before.

README (#1). I went a different way. The whole batch section came out, since the feature isn't rolled out. That means the homogeneous constraint isn't written down anywhere now, and neither is the header. Both need to land when this ships, along with a note about the batch-wide line cap once it does. The integration test posts a single entry type already.

ast.Name. Now ast.parse(..., mode="eval").body. You were right that unparse was hiding it. compile() on the old node gives TypeError: required field "ctx" missing from Name.

Encoding, directory. Both done.

Builtin generics. Neither hand-written codegen module imports typing any more. The emitted module still matches ariadne's style.

Annotations. Typing _unwrap_type turned up something. The type name it returned was thrown away by its only caller, which did _, required = .... That dead half was what forced the loose -> tuple in the first place. It's _is_required(type_node: TypeNode) -> bool now, and the stringly kind == "non_null_type" comparison became an isinstance check. Same treatment for kind == "int_value". Everything else on your list is in: ValueNode | None, set[str], typed parameters_node and method_def, field_node.

@cache. Swapped. The laziness still earns its keep, because it parses BASE_CLASS_SOURCE, which sits below it in the module.

description. I missed it. Added, and omitted from the payload when unset. lines stays out because LedgerEntryInput documents that it can't be combined with a typed entry.

Round-trip. Line added to the base class docstring, so it appears in every generated typed_entries.py. I looked at when_used="json" so python-mode dumps would stay reversible, but the base client dumps in python mode, so the wire shape has to come from there.

Silent fallbacks. Both warn now. Testing the entries one found something worse: the widening call is guarded on the operation name, so an upstream rename of addLedgerEntries skipped widening with no warning of any kind. There's a third warning for that, keyed on whether the operation was seen at all, which keeps the two cases from double-reporting. Neither path is reachable from the current queries. I kept both as warnings, since the failure costs a caller their type checking and leaves the wire payload correct.

poetry.toml. Not intentional. Deleted.

Tests. 4cc6201 adds the three that were missing, so all four of your asks are covered now and none of them need credentials. 29 tests total.

_safe_field_name gets a parametrized table: type, class, def, json, copy, model_dump, ik, posted, description all escape with a trailing underscore, and userId and captureAmount come out snake_cased. Then extract_entry_spec asserts the pairs that end up in PARAMETER_FIELDS, and render_module asserts they survive into the emitted source. The to_input tests check the nested shape, the wire keys, and what gets left out when a field was never set.

One thing worth passing on from writing them. My first version subclassed the committed fragment/sdk/typed_entries to get a TypedLedgerEntry to build on. Deleting description=self.description from the renderer left all 26 tests green, because that module is generated output and the tests were reading whenever codegen last ran. The fixture now executes BASE_CLASS_SOURCE directly with the input types injected into a namespace. After that change, four separate mutations to the renderer each fail: dropping description, sending the escaped Python name instead of the Schema name, keeping unset parameters, and flipping exclude_none.

Still open: the session-scoped fixture for the integration tests. Happy to do that next if you want it before this comes out of WIP.

[snoble]

🚫 DO NOT MERGE — approving to unblock, but the five inline items below need resolving first.

Second pass by Claude, running in Steven's session — these are Claude's words and judgements, not Steven's.

The idempotency fix is real: resolve_class_names is pure, and executing BASE_CLASS_SOURCE in the fixture instead of importing the committed module is the right way to keep those tests pointed at what codegen emits now rather than at whenever it last ran. Annotations, warnings, encoding, ast.parse — all in.

Five things I'd want resolved before merge, inline below. Three are code, one is a confirmation, one is docs.

Separately, and not something to action here: the test and typecheck holes. Nothing ever executes render_module's output (the per-entry classes are only string-matched), the optional-parameter path has no coverage in any snapshot, and CI runs mypy -p fragment only — so the new [tool.mypy] mypy_path is dead config and the PR's central typing claim is verified by nothing automated. Steven and I are picking those up in a stacked PR.

[snoble]

Blocking: this typechecks but breaks at runtime for any sequence that isn't a list.

ariadne's base client converts variables with an isinstance check on list, not Sequence:

def _convert_value(self, value):
    if isinstance(value, BaseModel): return value.model_dump(by_alias=True, exclude_unset=True)
    if isinstance(value, list):      return [self._convert_value(item) for item in value]
    return value

So a tuple satisfies the widened annotation, skips conversion, and dies in json.dumps. Verified against the snapshotted client:

list  -> {"entries": [{"entry": {...}, "ik": "a"}]}
tuple -> TypeError: Object of type OrderPlacedV1 is not JSON serializable

and mypy accepts add_ledger_entries(entries=(t,)) with no error. Before this PR the annotation was list[...], so a tuple was a type error and never reached the wire — the widening is what opens it.

The widening itself is right, and I confirmed it does what it claims: list[OrderPlacedV1] passes via covariance, mixed typed/raw lists pass, list[str] and a wrong parameter type are both rejected. The fix is to also rewrite the method body's variables assignment to {"entries": list(entries)} — this plugin is already doing AST surgery on that method.

[vigneshwerv]

Fixed in 5af71d4. Reproduced it first against the snapshotted client, same result you got.

The plugin now also rewrites the method body, so the generated assignment reads:

variables: dict[str, object] = {"entries": list(entries)}

Applied to fragment/sdk, fragment/sync_sdk and the snapshot. Tuple, generator, and a mixed tuple of typed plus raw all serialise after it.

The coercion lives next to the widening rather than somewhere else, since widening the annotation is what makes the failure reachable. There is a warning if that assignment ever stops matching, alongside the two already in this plugin.

test_generated_batch_method_coerces_entries_to_a_list parses the snapshot and asserts the dict literal is exactly {'entries': list(entries)}. Removing the coercion and regenerating makes it fail.

Vignesh and I did weigh dropping Sequence instead, since list[Union[...]] would reject the tuple at type-check time and need no rewrite. Measured what each accepts:

call	list[Union[...]]	Sequence[Union[...]]
inline literal	ok	ok
pre-built list[OrderPlacedV1]	rejected	ok
tuple	rejected	ok, with the coercion

The pre-built list is the common shape, a comprehension over orders infers list[OrderPlacedV1], so we kept Sequence.

[snoble]

Blocking: two Schema parameters that snake_case alike collide silently, and the wire payload is wrong.

resolve_class_names guards class-name collisions (auth_hold vs authHold), but nothing guards field names within a class. A Schema declaring both user_id and userId renders:

    PARAMETER_FIELDS: ClassVar[Dict[str, str]] = {
        "user_id": "user_id",
        "userId": "user_id",
    }

    user_id: str
    user_id: str

Pydantic accepts the duplicate declaration, the last one wins, and both wire keys get the same value. No warning anywhere. That's the same likelihood as the pascal-case collision you already handle, with a worse failure mode — wrong data rather than a missing model.

_safe_field_name can't see its siblings, so the disambiguation needs to happen where the parameter list is assembled (_extract_parameters), the way resolve_class_names does it for class names.

[vigneshwerv]

Fixed in 0d14d2c. Reproduced exactly as described, one value under both keys:

{"user_id": "VALUE", "userId": "VALUE"}

You were right about where it belongs. _safe_field_name cannot see its siblings, so the check is now in _extract_parameters where the list is assembled. Later collisions take a numeric suffix, the same shape resolve_class_names uses one level up, and a warning names the parameter that moved:

WARNING:console:Parameters in operation PostThing map to the same Python field
'user_id'; 'userId' is generated as 'user_id_2' instead. The wire payload is unaffected.

Result:

PARAMETER_FIELDS = {"user_id": "user_id", "userId": "user_id_2"}

{"user_id": "SNAKE", "userId": "CAMEL"}

Three tests: the two-way case asserting the exact pairs, a three-way case checking the counter keeps going, and one asserting the rendered PARAMETER_FIELDS keeps both Schema names. Reverting to the plain _safe_field_name call fails all three. The marketing-schema snapshot is unchanged, since none of its parameters collide.

[snoble]

Blocking: this plugin's position in the list is load-bearing and undocumented.

collect_annotations harvests annotations off method_def after RewriteUnsetTypeMethodArguments has rewritten Union[Optional[X], UnsetType] into Optional[X]. That only holds because GenerateTypedLedgerEntries is listed below it. Move it up and typed models emit UnsetType in their annotations, into a module that doesn't import it — NameError when the generated SDK is imported.

Same class of bug as the intra-plugin hook ordering you fixed, one level up. A comment here plus a guard in collect_annotations (warn and skip, or fail, on an annotation mentioning UnsetType) closes it.

[vigneshwerv]

Fixed in 78348c2. Moving the plugin above the rewriter reproduces it:

memo: Union[Optional[str], UnsetType] = None

and the generated SDK then dies on import with NameError: name 'Union' is not defined.

helpers.py now says the order is load-bearing and why. collect_annotations raises when an annotation still mentions UnsetType, using ariadne's own UNSET_TYPE_NAME:

RuntimeError: Annotation 'Union[Optional[str], UnsetType]' for argument 'memo' still
mentions UnsetType. GenerateTypedLedgerEntries must be listed after
RewriteUnsetTypeMethodArguments in the codegen plugin list; see get_codegen_config
in fragment/codegen/helpers.py.

I took the fail option rather than warn-and-skip. The Any fallback degrades into a working SDK with one untyped field, so a warning suits it. This one writes a package nobody can import, so continuing would hand someone a NameError a long way from the cause.

Two tests, one for the raise and one for the normal path returning {"memo": "Optional[str]"}.

[snoble]

Blocking on a confirmation, not on the code.

Normalising unpinned to 1 and always putting it on the wire is the option I'd pick too — but it's only correct if the API resolves a missing typeVersion to 1 rather than to the latest version of the entry type.

Every CLI-generated query in the snapshot pins typeVersion explicitly, so the blast radius is hand-written queries only. But if the server default is "latest", this silently pins those callers to V1 forever, and it's a behaviour change from the previous revision of this PR (which sent nothing). Worth confirming API-side rather than taking my earlier review's word for it.

[vigneshwerv]

Confirmed API-side: a missing typeVersion resolves to 1, never to the latest version. So normalising to 1 and putting it on the wire is equivalent to sending nothing, and the previous revision of this PR was equivalent too. Callers see no behaviour change, only a name that stops disagreeing with the payload.

Your blast-radius read matches the repo. All 9 CLI-generated operations in tests/template-schema/queries.graphql pin typeVersion explicitly, and every model in the snapshot carries one, so hand-written queries are the only place the normalisation does anything.

One consequence worth naming: because it happens at extraction, an unpinned operation and one pinning typeVersion: 1 now share an identity and collapse into a single model. That follows from the resolution rule, and it wasn't true before this revision.

[snoble]

Blocking: this is now the only user-facing documentation of the feature, and it describes a call the API rejects.

Pulling the README section resolved the wrong-examples problem by deletion, but this line survived it. A reader will do exactly what it says and get invalid_input_provided:

• addLedgerEntries enforces a homogeneous batch — one entry type, one type version, one ledger. Mixing forms (typed + raw) works; mixing types doesn't. The sentence isn't wrong, it's incomplete in the direction that fails, and the typed models make violating it feel natural.
• The endpoint requires headers={"X-Fragment-Experimental": "true"}. tests/test_add_ledger_entries.py passes it, so the tests pass and a reader following this doesn't.

Neither constraint is written down anywhere in the repo now. Either qualify the sentence here or land the docs before this ships.

[vigneshwerv]

Leaving the CHANGELOG as it is. This isn't blocking, because the PR merges once the API is rolled out to all users, so nothing here reaches anyone before the constraints are true and documented alongside the rollout.

The two constraints you named are right and worth capturing then: the homogeneous batch, and the X-Fragment-Experimental: true header. The batch-wide line cap you mentioned should join them once it lands.