fix(dojo-core): model keys serialization with Serde #3326
Conversation
|
ohayo, sensei! WalkthroughAdds enum-keyed test models and tests, updates model attribute macro to serialize keys per-member with legacy semantics, adjusts a layout utility to panic instead of returning None in a branch, and expands the test artifact rebuild script with formatting reordering, Katana provisioning, and test DB generation. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Macro as #[dojo::model] Macro
participant DF as DojoFormatter
Dev->>Macro: Expand model with members
loop For each member
alt Member is #[key]
Macro->>DF: serialize_member_ty(db, member, use_legacy=true, is_key=true)
Macro->>Macro: push to serialized_keys
Macro->>DF: deserialize_member_ty(db, member, use_legacy=true, source="keys")
else Non-key member (value)
Macro->>DF: serialize_member_ty(db, member, use_legacy=model.use_legacy_storage, is_key=false)
Macro->>Macro: push to serialized_values
Macro->>DF: deserialize_member_ty(db, member, use_legacy=model.use_legacy_storage, source="values")
end
end
Note over Macro: Keys always use legacy semantics
sequenceDiagram
participant C as Caller
participant L as find_model_field_layout
C->>L: Query field layout for model
alt Model is struct
L-->>C: Some(field layout)
else Not a struct
L--xC: panic("Model layout is not a struct")
end
Note over L: Removed None return path after panic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing Touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
crates/dojo/core/src/utils/layout.cairo (1)
30-30: ohayo, sensei — Docstring now lies; function panics instead of returning None.The else-branch is now a hard panic, so the “None otherwise” contract is no longer true. Please align the docs to avoid misleading callers.
Proposed doc tweak (outside selected lines):
-/// Some(Layout) if the field layout has been found, None otherwise. +/// Returns Some(Layout) if the field layout has been found. +/// Panics if `model_layout` is not a `Layout::Struct` (this should be unreachable for models).scripts/rebuild_test_artifacts.sh (1)
35-38: Make DB generation more robust.Consider guarding these steps with
set -euo pipefail(proposed above) so failures propagate, and optionally echo where the DB is written for easier debugging.crates/dojo/macros/src/attributes/model.rs (1)
111-118: Optional: reduce the double-walk of members.You iterate
struct_ast.members(...)for (de)serialization and later re-iterate parsed members to build metadata. You could consolidate into one pass that accumulates both codegen snippets and metadata to shave complexity, but not required.Also applies to: 126-132, 133-139
crates/dojo/core-tests/src/tests/model/model.cairo (2)
724-780: Strong coverage for legacy enum-key path; tiny typo nit.Assertions match expected Serde encodings and legacy defaults. Please fix “unitialized” → “uninitialized”.
Patch within this hunk:
- // read unitialized model, write and read back + // read uninitialized model, write and read back @@ - "LegacyModelWithEnumKey: read unitialized model failed", + "LegacyModelWithEnumKey: read uninitialized model failed",
783-840: DojoStore enum-key path covered; repeat the typo fix.Encodings and defaults look correct for DojoStore. Please fix the same typo here.
Patch within this hunk:
- // read unitialized model, write and read back + // read uninitialized model, write and read back @@ - "DojoStoreModelWithEnumKey: read unitialized model failed", + "DojoStoreModelWithEnumKey: read uninitialized model failed",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
spawn-and-move-db.tar.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
crates/dojo/core-tests/src/tests/model/model.cairo(3 hunks)crates/dojo/core/src/utils/layout.cairo(1 hunks)crates/dojo/macros/src/attributes/model.rs(1 hunks)scripts/rebuild_test_artifacts.sh(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
crates/dojo/macros/src/attributes/model.rs (1)
crates/dojo/macros/src/helpers/formatter.rs (2)
serialize_member_ty(22-30)deserialize_member_ty(45-58)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: fmt
🔇 Additional comments (10)
scripts/rebuild_test_artifacts.sh (3)
15-18: Formatting reordering LGTM.Running formatters right after abigen is sensible and harmless.
21-24: Good: broader artifact rebuild coverage.Adding the release profile and core-tests invocation improves fidelity.
25-33: Harden Katana bootstrap and fail fastohayo sensei, apply these changes and double-check runner behavior:
- Within the existing hunk, replace
cpwith:- cp "$(command -v katana)" /tmp/katana + install -m0755 "$(command -v katana)" /tmp/katana- At the top of
scripts/rebuild_test_artifacts.sh, add:+set -euo pipefail- Verify that
xtask/generate-test-db(via KatanaRunner) actually invokes/tmp/katana—e.g. prepend/tmpto$PATHor setKATANA_RUNNER_BIN=/tmp/katanabefore running.crates/dojo/macros/src/attributes/model.rs (3)
111-118: ohayo, sensei — Forcing Serde for keys is correct and matches the PR goal.This ensures stable key encoding across legacy and DojoStore. Looks good.
119-125: Serde-based key deserialization LGTM.Using the "keys" input and Serde here aligns with the serialization path.
126-132: Values follow storage semantics — clean separation.Nice split: values use
model.use_legacy_storagewhile keys stay Serde.Also applies to: 133-139
crates/dojo/core-tests/src/tests/model/model.cairo (4)
109-116: ohayo, sensei — EnumKey definition LGTM.Derives (including Default and DojoStore) are appropriate for exercising both storage modes.
117-128: LegacyModelWithEnumKey test model LGTM.Covers enum key + mixed value shapes under legacy semantics.
129-140: DojoStoreModelWithEnumKey test model LGTM.Validates the key/value split across DojoStore semantics.
222-224: Namespace registration LGTM.Models are properly added to the test world resources.
Description
Whatever if a model is a legacy one or a DojoStore one, model keys must always be serialized with Serde.
Tests
Added to documentation?
Checklist
scripts/rust_fmt.sh,scripts/cairo_fmt.sh)scripts/clippy.sh,scripts/docs.sh)Summary by CodeRabbit