Repository navigation
refactor(api): replace json.loads with Pydantic validation in retention and website layers- #43588 - #44170
Open
haloneko wants to merge 8 commits into
Open
refactor(api): replace json.loads with Pydantic validation in retention and website layers- #43588#44170haloneko wants to merge 8 commits into
haloneko wants to merge 8 commits into
Conversation
…ived_workflow_run Replace json.loads plus the handwritten required-field/tables checks with a TypeAdapter over the existing _ArchiveManifest TypedDict, so manifest shape and field types are validated in one pass. Business rules that depend on call arguments (schema_version, workflow_run_id, tenant_id, app_id, workflow_id matching and the archived table list) stay in Python, since Pydantic cannot see them. Part of langgenius#33092
…chive_maintenance Replace json.loads plus the handwritten required-field and table-entry checks with a Pydantic model, so manifest shape and field types are validated in one pass. BundleManifest declares eighteen fields but only thirteen are required: the five range/audit fields are absent from older manifests, so they stay optional to keep the current validation strictness instead of tightening it. Unknown fields are preserved via extra='allow' to match the previous json.loads behaviour. Business rules that depend on the object_prefix argument (schema/format values, prefix match, tenant prefix, run_ids length, per-table object_key) stay in Python. Add tests pinning the mandatory/optional split and the new type validation. Part of langgenius#33092
…id_plan_workflow_run
Replace json.loads plus the handwritten dict checks in _load_bundle_index and _build_bundle_index with Pydantic models.
The shard index model mirrors ArchiveBundleIndexDict; campaign_ids defaults to an empty list rather than None because callers do set(index.get('campaign_ids', [])) and set(None) would raise. The manifest model requires only the four fields _build_bundle_index actually reads, so older manifests that lack the later range/audit fields still contribute to a rebuilt index.
Business rules that depend on identity (schema/format values and shard prefix match) stay in Python.
This module had no unit tests; add one covering both entry points.
Part of langgenius#33092
…ydantic Replace the last two json.loads calls in the claimed scope with Pydantic validation. website_service: FirecrawlDocumentData is a total TypedDict, but cached payloads were written before some of its fields existed, so a model with None defaults is used instead of rejecting them. Documents are dumped with exclude_unset so the returned dict matches the cached payload rather than gaining null entries. message_export_service: only etriever_resources is read from message_metadata, so the model declares just that field and ignores the rest. The tolerant except is kept as (ValidationError, TypeError) because a non-string column must still yield an empty list rather than fail the export. Part of langgenius#33092
- Type retriever resources as list[dict[str, JsonValue]] instead of list[Any], matching how installed_app_message_service already handles the same message_metadata payload. - Drop the TypeAdapter wrappers whose target is a BaseModel: ArchiveBundleIndexModel, ShardManifestEntryModel and _MessageMetadataModel can all call model_validate_json directly. TypeAdapter stays only where the target is not a BaseModel, i.e. the _ArchiveManifest TypedDict and list[_CachedFirecrawlDocument]. Part of langgenius#33092
Validate the V2 bundle manifest, the shard index and the cached Firecrawl documents directly against the TypedDicts the code already declares, instead of routing through throwaway BaseModel classes and casting the result back: - remove three cast() calls together with the matching model_dump() round-trips, which produced a plain dict[str, Any] and erased the type information - drop the now-unused TypeVar imports - reuse the existing BundleManifest, ArchiveBundleIndexDict and FirecrawlDocumentData definitions, so the validated shape has a single source of truth - keep the five range/audit manifest fields NotRequired, so validation stays as permissive as the handwritten check it replaces Rewrite the affected test fixtures so they match the shapes the code actually writes and reads: fill in the range/audit fields of the bundle manifest fixture, add the description and markdown fields to the stored Firecrawl document fixture, and rename the optional-range-field test accordingly. Part of langgenius#33092
The false side of the campaign_id check in _build_bundle_index was never taken: the happy-path test supplies a campaign_id and the error-path tests raise before reaching it, so Codecov counted the branch against the patch. Part of [langgenius#33092](langgenius#33092)
ghfind-review
Bot
added
the
review: medium
github-actions
Bot
added
the
size:L
Contributor
Pyrefly Type Coverage
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #44170 +/- ##
==========================================
+ Coverage 88.93% 88.94% +0.01%
==========================================
Files 5287 5287
Lines 324064 324054 -10
Branches 63702 63687 -15
==========================================
+ Hits 288192 288226 +34
+ Misses 30694 30662 -32
+ Partials 5178 5166 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
size:XL
haloneko
marked this pull request as draft
October 15, 2026 14:51
- use contextlib.suppress for the message_metadata validation guard (ruff suppressible-exception) - replace the net-new Mock/MagicMock/SimpleNamespace test doubles with real in-memory storage and stub classes (check_no_new_test_doubles)
haloneko
force-pushed
the
refactor/pydantic-inspector-message
branch
from
October 15, 2026 14:59
f58a7ee to
b3804cf
Compare
haloneko
marked this pull request as ready for review
October 15, 2026 15:01
Contributor
Author
|
@asukaminato0721 could you please review this PR? Thanks! |
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
1 participant
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.
Important
Fixes #<issue number>.Summary
Replaces json.loads plus handwritten field checks with Pydantic validation at six call sites, so manifest / index / cache shapes and field types are validated in one pass instead of by ad-hoc isinstance and set-difference checks. Validation reuses the TypedDicts the code already declares, so each validated shape has a single source of truth.
The four logical changes span seven commits so each can be reverted independently:
Two follow-up commits (simplify validation adapters per review, drop redundant casts by validating existing TypedDicts) drop the throwaway BaseModel wrappers and the cast() / model_dump() round-trips in favour of the existing TypedDicts, and a test-only commit (test(api): cover the bundle manifest branch without campaign_id) closes the remaining branch coverage.
api/services/retention/workflow_run/archive_bundle_index.py still decodes manifests with json.loads; it is intentionally out of scope for this PR.
Business rules that depend on call arguments (schema/format values, tenant and prefix matching, run_ids length, per-table object_key) stay in Python, since Pydantic cannot see them. Validation targets the existing total/NotRequired TypedDicts, so validated results are only read through declared fields; unknown keys are dropped, which is behaviour-neutral at these call sites. ShardManifestEntryModel keeps extra="allow" because it is consumed as a model rather than a plain dict, and _MessageMetadataModel uses extra="ignore" deliberately.
Tests
From CodeBuddy
Screenshots
Checklist
make lint && make type-check(backend) andvp staged(frontend) to appease the lint godsPart of #33092