-
Notifications
You must be signed in to change notification settings - Fork 11
refactor: standardize SDK JSON serialization on orjson #1185
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Draft
dgarros
wants to merge
17
commits into
infrahub-develop
Choose a base branch
from
dga/feat-orjson-pd5o6
base: infrahub-develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+896
−227
Draft
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
234c440
docs(specs): add orjson JSON standardization spec
dgarros a586a53
docs(specs): add orjson migration plan artifacts
dgarros 618428d
docs(specs): add orjson migration critique + apply recommendations
dgarros 95067ff
docs(specs): add orjson migration tasks.md
dgarros e21d4e2
docs(specs): record alignment check (skipped, no source PRD)
dgarros 7d14a09
build(deps): add orjson dependency
dgarros 015e1ef
feat(utils): migrate dict_hash and decode_json to orjson
dgarros 9a0d366
feat(sdk): migrate encode sites to orjson
dgarros 37b2278
refactor: migrate decode sites to orjson.loads
dgarros 9be1b45
feat(orjson): migrate recorder, playback, and json exporter to orjson
dgarros 814fa68
refactor(pytest-plugin): migrate items JSON serialization to orjson
dgarros 58320e0
test(json): pin orjson migration guarantees
dgarros d887aca
chore(deps): remove ujson, complete orjson migration
dgarros b3b3921
docs(specs): add orjson implementation report
dgarros 484c0b5
ci: fix markdown-lint MD050 in orjson implementation report
dgarros 9020cfd
fix(orjson): preserve non-str-key coercion at arbitrary-data sites
dgarros b5d4e4b
docs(orjson): document non-ASCII output change and known limitations
dgarros File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| Standardized JSON serialization across the SDK on `orjson`, replacing `ujson` and stdlib `json`. Behaviour is preserved for ASCII data, with two categories of change for non-ASCII data and legacy on-disk artifacts: | ||
|
|
||
| - **Non-ASCII characters are now emitted as raw UTF-8** rather than `\uXXXX` escapes, across all JSON the SDK writes (CLI output, `infrahubctl validate --out`, telemetry and transfer exports, check logs, and pytest failure messages). The output is still valid JSON and round-trips correctly through the SDK; only its exact bytes differ, which may affect external tools that byte-compare or checksum these files. | ||
| - **The tracking-group name** derived from query parameters containing non-ASCII characters shifts once on upgrade (same raw-UTF-8 cause). The previously-created group is orphaned and a new one is created; cleanup of the orphaned group is manual. | ||
| - **Playback recordings created by an earlier SDK version** may no longer be located, because the recorded request body is now serialized compactly as raw UTF-8, changing the content hash used in the recording filename. Re-record fixtures with this version. | ||
|
|
||
| Known limitations of the new serializer, unchanged from `orjson`'s defaults: the `from_json` Jinja filter returns a `float` for integers larger than 64 bits (previously an exact `int`), and JSON decoding accepts UTF-8 input only (the Infrahub API always responds in UTF-8). | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # Spec/Ask Alignment Check | ||
|
|
||
| **Source**: None. The `/speckit-opsmill-prep` invocation had empty input; the spec was seeded from the in-session idea brief produced by the grilling interview, not from an external PRD (no URL, no inline requirements document). | ||
|
|
||
| **Verdict**: ⚠️ SKIPPED — no source-of-truth PRD to align against. | ||
|
|
||
| **Findings**: N/A (nothing to compare). | ||
|
|
||
| **Action**: Proceed. The spec's fidelity rests on the grilling-phase decisions captured directly in this session (scope = orjson-only; dict_hash non-ASCII drift accepted; datetime passthrough; parity-only success bar; dependency swap confirmed). Those decisions are reflected in spec.md and were validated by the Phase 3 critique. |
35 changes: 35 additions & 0 deletions
35
dev/specs/002-orjson-json-migration/checklists/requirements.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Specification Quality Checklist: Standardize SDK JSON serialization on orjson | ||
|
|
||
| **Purpose**: Validate specification completeness and quality before proceeding to planning | ||
| **Created**: 2026-07-14 | ||
| **Feature**: [spec.md](../spec.md) | ||
|
|
||
| ## Content Quality | ||
|
|
||
| - [x] No implementation details (languages, frameworks, APIs) | ||
| - [x] Focused on user value and business needs | ||
| - [x] Written for non-technical stakeholders | ||
| - [x] All mandatory sections completed | ||
|
|
||
| ## Requirement Completeness | ||
|
|
||
| - [x] No [NEEDS CLARIFICATION] markers remain | ||
| - [x] Requirements are testable and unambiguous | ||
| - [x] Success criteria are measurable | ||
| - [x] Success criteria are technology-agnostic (no implementation details) | ||
| - [x] All acceptance scenarios are defined | ||
| - [x] Edge cases are identified | ||
| - [x] Scope is clearly bounded | ||
| - [x] Dependencies and assumptions identified | ||
|
|
||
| ## Feature Readiness | ||
|
|
||
| - [x] All functional requirements have clear acceptance criteria | ||
| - [x] User scenarios cover primary flows | ||
| - [x] Feature meets measurable outcomes defined in Success Criteria | ||
| - [x] No implementation details leak into specification | ||
|
|
||
| ## Notes | ||
|
|
||
| - The spec deliberately avoids naming the specific JSON libraries in requirement/success statements to keep them technology-agnostic; the concrete library choice (orjson, replacing ujson + stdlib json) is recorded as an input/decision and belongs to the plan phase. | ||
| - Single P1 user story by design: the migration is atomic and cannot be meaningfully split into independently-shippable slices without creating the two-library intermediate state the work exists to remove. |
32 changes: 32 additions & 0 deletions
32
dev/specs/002-orjson-json-migration/contracts/serialization-contract.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| # Serialization behavioral contract | ||
|
|
||
| This library exposes no new public API. The contract this migration MUST preserve is behavioral — what existing callers and consumers observe. | ||
|
|
||
| ## Preserved (MUST NOT change) | ||
|
|
||
| 1. **`infrahub_sdk.utils.dict_hash(dict) -> str`** — signature and return type unchanged. Returns the same MD5 hex string as before for any input whose values are ASCII strings, integers, floats, booleans, `None`, or nested dicts/lists thereof. | ||
| - Committed vectors that MUST still hold: | ||
| - `{"a": 1, "b": 2}` → `608de49a4600dbb5b173492759792e4a` | ||
| - `{"b": 2, "a": {"c": 1, "d": 2}}` → `4d8f1a3d03e0b487983383d0ff984d13` | ||
| - `{}` → `99914b932bd37a50b983c5e7c90ae93b` | ||
|
|
||
| 2. **CLI JSON output** (`infrahubctl` via `ctl/formatters/json.py`, `ctl/validate.py`, `ctl/cli_commands.py`) — same indentation (2 spaces) and same textual rendering of date/time values (`str()` form, space separator) as before. | ||
|
|
||
| 3. **Decode-error handling** — every input that previously raised a caught JSON decode error still raises one that is caught at the same site (no `except` gap). | ||
|
|
||
| 4. **Serialized payloads sent to the API** (`graphql/multipart.py`, `graphql/renderers.py`) — semantically equivalent JSON; multipart form fields remain `str`. | ||
|
|
||
| 5. **Recorded fixtures** (`recorder.py` → `playback.py`) — a file written by the recorder is readable by playback and yields the same decoded object. | ||
|
|
||
| ## Allowed to change (documented) | ||
|
|
||
| 1. **`dict_hash` for non-ASCII values** — returns a different (but stable) hash than the previous library. Consequence: the persisted query-group name derived from such params changes once on upgrade. Pinned by a new test vector; noted in release notes. | ||
|
|
||
| 2. **Human-facing pretty-print width** — debug prints and pytest failure/diff output change from 4-space to 2-space indentation. Not machine-consumed, not compared. | ||
|
|
||
| ## Contract tests | ||
|
|
||
| - `tests/unit/sdk/test_utils.py::test_dict_hash` — retains the three vectors above; adds a non-ASCII vector asserting the new orjson value. | ||
| - Existing CLI/formatter unit tests — pass unchanged (parity check for items 2 & 4). | ||
| - A record→replay round-trip test — covers item 5. | ||
| - A decode-of-invalid-input test — covers item 3. |
46 changes: 46 additions & 0 deletions
46
dev/specs/002-orjson-json-migration/critiques/critique-20260714.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Critique: Standardize SDK JSON serialization on orjson | ||
|
|
||
| **Date**: 2026-07-14 · **Artifacts reviewed**: spec.md, plan.md, research.md, data-model.md, contracts/serialization-contract.md | ||
|
|
||
| ## Executive Summary | ||
|
|
||
| The spec and plan are sound and implementation-ready. This is a well-scoped, mechanical, behaviour-preserving migration with an explicit parity bar, a documented single behavioural change (non-ASCII query-group naming), and existing test coverage at the parity-critical sites (`tests/unit/ctl/formatters/test_json.py`, `tests/unit/sdk/test_file_object.py`). No must-address (🎯) findings. Several recommendations (💡) were applied to spec/plan to close small correctness and verification gaps surfaced by the engineering lens. **Verdict: ✅ PROCEED (with updates applied).** | ||
|
|
||
| ## Product Lens Findings | ||
|
|
||
| - **3a Problem Validation** — Clear, evidenced by two tracked tech-debt issues (dual-library ambiguity + decode-error hazard). No concern. | ||
| - **3b User Value** — Value is twofold (perf on the hot path; contributor clarity). *P1 (💡)*: the feature is perf-motivated but SC-003 sets a parity-only bar with no measurement, so the SDK ships a performance change with no evidence of the win. This is a conscious decision (no benchmark harness in scope); recommend capturing one ad-hoc before/after number in the PR description. Applied as a note in the plan. | ||
| - **3c Alternatives** — Single-library-on-orjson correctly supersedes "standardize then migrate." No concern. | ||
| - **3d Edge Cases / Migration** — *P2 (💡)*: users with non-ASCII query params get an orphaned tracking group on upgrade. Spec documents the change (SC-004) but offered no user guidance. Applied: added an explicit out-of-scope note that stale-group cleanup is manual and belongs to release notes. | ||
| - **3e Success Measurement** — Success criteria are measurable and verifiable (grep counts, test-suite green, byte-identical output). Rollback for a library = revert the change. No concern. | ||
|
|
||
| ## Engineering Lens Findings | ||
|
|
||
| - **4a Architecture** — Uniform site-by-site substitution against a fixed 9-row transform table; no structural change. Sound and testable. | ||
| - **4b Failure Modes** — | ||
| - *E1 (💡, investigated → not a blocker)*: orjson natively serializes `Enum`/dataclass/numpy where `default=str` would otherwise stringify them, which could break the byte-parity claim at `ctl/formatters/json.py`. Investigation of `extract_node_data`/`extract_node_detail` shows the value domain is primitives + `datetime` (covered by `OPT_PASSTHROUGH_DATETIME`) + str-rendered IP/UUID (same output). No Python `Enum`/dataclass objects flow through. Recommend a characterization test asserting a datetime-bearing node renders identically. Applied to spec (FR-002 acceptance) + plan. | ||
| - *E2 (💡)*: orjson `loads` **rejects** `NaN`/`Infinity`/`-Infinity` that stdlib accepted, and orjson `dumps(nan)` emits `null` where stdlib emitted invalid `NaN`. Both are non-standard JSON and unlikely in Infrahub API traffic, but it is a real decode/encode behaviour change. Applied: added as an explicit edge case in the spec. | ||
| - **4c Security & Privacy** — No new trust boundary; orjson (Rust/serde_json) is a mature, widely-used, permissively-licensed (Apache-2.0/MIT) dependency. No concern. | ||
| - **4d Performance & Scalability** — Strictly improves the hot path; no new bottleneck. No concern. | ||
| - **4e Testing** — Parity-critical sites have existing unit tests (formatters, file-object). *E3 (💡)*: pytest-plugin failure/diff output shifts 4→2 space; if any test asserts that exact text it will break. Applied: flagged in plan as a site to check during implementation; treat as characterization update, not a contract break. | ||
| - **4f Operational Readiness** — N/A (library); change is atomic and revertible. | ||
| - **4g Dependencies** — *E4 (💡)*: the "wheels for 3.10–3.14" assumption is stated but unverified locally. orjson 3.11.x publishes cp314 wheels; the CI matrix will confirm across interpreters. Applied: assumption reworded to "confirmed by the CI test matrix." | ||
|
|
||
| ## Cross-Lens Insights | ||
|
|
||
| - *X1*: The migration's value/risk both rest on assumptions the plan now makes explicit and cheap to verify — perf (measure once in PR), type-parity (one characterization test), wheel coverage (CI matrix). None warrant blocking; all are captured. | ||
|
|
||
| ## Findings Summary Table | ||
|
|
||
| | ID | Lens | Severity | Category | Finding | Suggestion | Status | | ||
| |----|------|----------|----------|---------|------------|--------| | ||
| | P1 | Product | 💡 | User Value | Perf-motivated but unmeasured | Capture one ad-hoc before/after in PR | Applied (plan note) | | ||
| | P2 | Product | 💡 | Migration | Non-ASCII group orphaning lacks user guidance | Note manual cleanup in release notes | Applied (spec out-of-scope) | | ||
| | E1 | Eng | 💡 | Failure Modes | orjson native special-type handling vs default=str | Characterization test on datetime node | Applied (FR-002) | | ||
| | E2 | Eng | 💡 | Failure Modes | orjson rejects NaN/Infinity on decode; nan→null on encode | Document as edge case | Applied (spec edge case) | | ||
| | E3 | Eng | 💡 | Testing | pytest-plugin 4→2 indent may break asserted text | Check/adjust during implementation | Applied (plan note) | | ||
| | E4 | Eng | 💡 | Dependencies | 3.14 wheel coverage unverified locally | Confirm via CI matrix | Applied (assumption reworded) | | ||
|
|
||
| ## Verdict | ||
|
|
||
| ✅ **PROCEED** — no must-address items. Recommendations applied to spec.md and plan.md. Ready for `/speckit-tasks`. |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| # Data Model: orjson migration | ||
|
|
||
| No new domain entities. This migration transforms serialization call sites. The "data model" here is the exhaustive call-site migration map — the source of truth for task generation. | ||
|
|
||
| ## No new entities | ||
|
|
||
| The only data concept indirectly affected is the **query-group name**: an existing, persisted identifier partly derived from `dict_hash(params)`. Its structure and public type (`str`) are unchanged; only its value changes, and only for non-ASCII parameter values (see spec Edge Cases). | ||
|
|
||
| ## Call-site migration map | ||
|
|
||
| Legend for transform: **D**=decode `.decode()` added · **OPT**=option flags · **EXC**=except-clause change · **FILE**=file-object rewrite · **COSMETIC**=4→2 indent width. | ||
|
|
||
| ### Encode sites (`dumps` / `dump`) | ||
|
|
||
| | File:line | Current | Transform | | ||
| |---|---|---| | ||
| | `client.py:218` | `ujson.dumps(variables, indent=4)` (debug print) | `OPT_INDENT_2`, **D**, **COSMETIC** | | ||
| | `checks.py:112` | `print(ujson.dumps(log_message))` | **D** | | ||
| | `ctl/validate.py:107` | `ujson.dumps(response, indent=2, sort_keys=True)` | `OPT_INDENT_2 \| OPT_SORT_KEYS`, **D** | | ||
| | `ctl/cli_commands.py:355` | `ujson.dumps(result, indent=2, sort_keys=True)` | `OPT_INDENT_2 \| OPT_SORT_KEYS`, **D** | | ||
| | `ctl/telemetry.py:127` | `json.dumps(snapshots, indent=2)` → `write_text` | `OPT_INDENT_2`, **D** | | ||
| | `ctl/formatters/json.py:42,59` | `json.dumps(x, indent=2, default=str)` | `OPT_INDENT_2 \| OPT_PASSTHROUGH_DATETIME`, `default=str`, **D** | | ||
| | `graphql/multipart.py:46,60` | `ujson.dumps(operations)` (str for httpx) | **D**, `OPT_NON_STR_KEYS` | | ||
| | `graphql/renderers.py:54` | `json.dumps(value)` (string escaping) | **D** | | ||
| | `utils.py:253` | `ujson.dumps(dictionary, sort_keys=True).encode()` | `OPT_SORT_KEYS` (already bytes; drop `.encode()`) | | ||
| | `playback.py:52` | `str(json.dumps(payload)).encode("UTF-8")` | `orjson.dumps(payload)` (already bytes; drop wrappers) | | ||
| | `recorder.py:59` | `ujson.dump(data, fobj, indent=4, sort_keys=True)` | `OPT_INDENT_2 \| OPT_SORT_KEYS`, **FILE**, **COSMETIC** | | ||
| | `transfer/exporter/json.py:151,155,166` | `ujson.dumps(...)` → `write_text` | **D**, `OPT_NON_STR_KEYS` | | ||
| | `pytest_plugin/items/check.py:52` | `ujson.dumps(resp.json(), indent=4)` | `OPT_INDENT_2`, **D**, **COSMETIC** | | ||
| | `pytest_plugin/items/graphql_query.py:31` | same | `OPT_INDENT_2`, **D**, **COSMETIC** | | ||
| | `pytest_plugin/items/python_transform.py:54` | same | `OPT_INDENT_2`, **D**, **COSMETIC** | | ||
| | `pytest_plugin/items/jinja2_transform.py:63` | `... indent=4, sort_keys=True` | `OPT_INDENT_2 \| OPT_SORT_KEYS`, **D**, **COSMETIC** | | ||
| | `pytest_plugin/items/base.py:62,63` | `ujson.dumps(x, indent=4, sort_keys=True).splitlines()` | `OPT_INDENT_2 \| OPT_SORT_KEYS`, **D**, **COSMETIC** | | ||
|
|
||
| ### Decode sites (`loads` / `load` / `response.json()`) | ||
|
|
||
| | File:line | Current | Transform | | ||
| |---|---|---| | ||
| | `utils.py:97` | `except json.decoder.JSONDecodeError` on `response.json()` | switch decode to `orjson.loads(response.content)`; **EXC** → `orjson.JSONDecodeError` | | ||
| | `schema/__init__.py:277` | `except json.decoder.JSONDecodeError` | same strategy; **EXC** | | ||
| | `ctl/parsers.py:27,28` | `json.loads(stripped)` + `except json.JSONDecodeError` | `orjson.loads`, **EXC** | | ||
| | `template/infrahub_filters.py:167,168` | `json.loads(value)` + `except (json.JSONDecodeError, TypeError)` | `orjson.loads`, **EXC** (keep `TypeError`) | | ||
| | `playback.py:57` | `ujson.load(fobj)` | `orjson.loads(fobj.read())`, **FILE** | | ||
| | `transfer/importer/json.py:67,150` | `ujson.loads(...)` | `orjson.loads(...)` | | ||
| | `pytest_plugin/models.py:57` | `ujson.loads(text)` | `orjson.loads(text)` | | ||
| | `pytest_plugin/items/{check,graphql_query,python_transform,jinja2_transform}.py` | `except ujson.JSONDecodeError` | **EXC** → matching orjson exception for the paired decode | | ||
|
|
||
| ### Import + dependency changes | ||
|
|
||
| | File | Change | | ||
| |---|---| | ||
| | all 21 modules above | replace `import ujson` / `import json` with `import orjson` (remove both legacy imports) | | ||
| | `utils.py`, `playback.py` | remove the **dual** import (both `json` and `ujson`) | | ||
| | `pyproject.toml` | `- "ujson>=5"`, `+ "orjson>=3.10"`; dev: `- "types-ujson"` | | ||
| | `uv.lock` | refresh | | ||
|
|
||
| ## Validation rules (from requirements) | ||
|
|
||
| - Zero `import ujson` / `import json` remain in `infrahub_sdk/` (FR-001). | ||
| - `indent=2` and CLI-formatter output byte-identical; datetime text form unchanged (FR-002). | ||
| - `dict_hash` unchanged for the 3 vectors + new non-ASCII vector pinned (FR-003). | ||
| - No bytes leak where str is expected (FR-004). | ||
| - Record→replay round-trip passes (FR-005). | ||
| - Malformed JSON still caught at every current site (FR-006). | ||
| - Manifest/lock reflect exactly the add/remove (FR-007). | ||
| - Int-keyed export payloads still serialize (FR-008). |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.