feat: introduce default values#347
Conversation
📝 WalkthroughWalkthroughAdds map-level ChangesData Schema & Test Fixtures
Database Layer Defaults System
Unit Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@goodmap/db.py`:
- Line 733: The lookup using next((p for p in raw_data["data"] if p["uuid"] ==
uuid), None) will raise KeyError if any row lacks the "uuid" key; change the
predicate to use p.get("uuid") == uuid so the scan is resilient to missing keys
(update the expression that assigns point and any similar lookups over
raw_data["data"] to use .get("uuid") instead of p["uuid"]).
- Around line 708-711: Guard the template matching loop against malformed shapes
by first validating types and handling unhashable values: ensure
map_data.get("data_templates", {}) yields dict-like entries and that each
variants is a dict before using "value in variants", and only call
type_defaults.update(variants[value]) if variants[value] is also a dict. In
practice, around the loop over map_data.get("data_templates", {}) check
isinstance(variants, dict), wrap the membership test in a try/except TypeError
to catch unhashable point values, and verify isinstance(variants.get(value),
dict) before calling type_defaults.update to avoid runtime errors from non-dict
template payloads.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5c556eb-b3fd-4f37-ba04-916a59e93a33
📒 Files selected for processing (3)
examples/e2e_test_data.jsongoodmap/db.pytests/unit_tests/test_db.py
| for field, variants in map_data.get("data_templates", {}).items(): | ||
| value = point.get(field) | ||
| if value is not None and value in variants: | ||
| type_defaults.update(variants[value]) |
There was a problem hiding this comment.
Guard template matching against malformed template/value shapes.
value in variants / variants[value] can raise at runtime (e.g., unhashable discriminator values or non-dict template payloads), which would fail the whole read path.
Proposed fix
- for field, variants in map_data.get("data_templates", {}).items():
- value = point.get(field)
- if value is not None and value in variants:
- type_defaults.update(variants[value])
+ for field, variants in map_data.get("data_templates", {}).items():
+ if not isinstance(variants, dict):
+ continue
+ value = point.get(field)
+ if value is None:
+ continue
+ try:
+ matched_template = variants.get(value)
+ except TypeError:
+ continue
+ if isinstance(matched_template, dict):
+ type_defaults.update(matched_template)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@goodmap/db.py` around lines 708 - 711, Guard the template matching loop
against malformed shapes by first validating types and handling unhashable
values: ensure map_data.get("data_templates", {}) yields dict-like entries and
that each variants is a dict before using "value in variants", and only call
type_defaults.update(variants[value]) if variants[value] is also a dict. In
practice, around the loop over map_data.get("data_templates", {}) check
isinstance(variants, dict), wrap the membership test in a try/except TypeError
to catch unhashable point values, and verify isinstance(variants.get(value),
dict) before calling type_defaults.update to avoid runtime errors from non-dict
template payloads.
| """ | ||
| point = next((point for point in raw_data["data"] if point["uuid"] == uuid), None) | ||
| return location_model.model_validate(point) if point else None | ||
| point = next((p for p in raw_data["data"] if p["uuid"] == uuid), None) |
There was a problem hiding this comment.
Avoid hard failure when one row is missing uuid.
Using p["uuid"] can throw KeyError and abort lookup even if a valid matching row exists later. Use .get("uuid") for resilient scanning.
Proposed fix
- point = next((p for p in raw_data["data"] if p["uuid"] == uuid), None)
+ point = next((p for p in raw_data["data"] if p.get("uuid") == uuid), None)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| point = next((p for p in raw_data["data"] if p["uuid"] == uuid), None) | |
| point = next((p for p in raw_data["data"] if p.get("uuid") == uuid), None) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@goodmap/db.py` at line 733, The lookup using next((p for p in
raw_data["data"] if p["uuid"] == uuid), None) will raise KeyError if any row
lacks the "uuid" key; change the predicate to use p.get("uuid") == uuid so the
scan is resilient to missing keys (update the expression that assigns point and
any similar lookups over raw_data["data"] to use .get("uuid") instead of
p["uuid"]).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit_tests/test_db.py (1)
328-328: ⚡ Quick winConsolidate repeated
create_location_modelsetup and use snake_case local name.These tests repeatedly build the same model and use
Locationas a local variable, which is noisy and keeps triggering lint warnings. A small fixture/helper (e.g.,location_model) would reduce duplication and improve readability.♻️ Suggested refactor pattern
+@pytest.fixture +def location_model(): + return create_location_model([], {}) + def test_json_db_add_location(): - Location = create_location_model([], {}) + Location = location_model db = Json({"data": []})Or rename all locals directly:
-Location = create_location_model([], {}) +location_model = create_location_model([], {})Also applies to: 337-337, 342-342, 351-351, 360-360, 368-368, 390-390, 404-404, 418-418, 429-429, 887-887, 1087-1087, 1104-1104, 1117-1117, 1134-1134, 1152-1152, 1169-1169, 1766-1766, 1784-1784, 1806-1806, 1970-1970, 2066-2066, 2099-2099, 2136-2136, 2153-2153, 2170-2170, 2175-2175, 2233-2233, 2257-2257, 2274-2275, 2280-2280, 2295-2295, 2305-2305, 2353-2353, 2374-2374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_db.py` at line 328, Many tests repeatedly call create_location_model([], {}) and assign it to a local named Location, causing duplication and lint warnings; refactor by adding a reusable pytest fixture or helper named location_model that returns create_location_model([], {}), and update each test to use the fixture (or replace local variable name Location with snake_case location) — look for usages of create_location_model and the local symbol Location in tests (e.g., lines referenced) and replace with either the injected fixture location_model or a local variable named location to satisfy linting and reduce duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit_tests/test_db.py`:
- Line 328: Many tests repeatedly call create_location_model([], {}) and assign
it to a local named Location, causing duplication and lint warnings; refactor by
adding a reusable pytest fixture or helper named location_model that returns
create_location_model([], {}), and update each test to use the fixture (or
replace local variable name Location with snake_case location) — look for usages
of create_location_model and the local symbol Location in tests (e.g., lines
referenced) and replace with either the injected fixture location_model or a
local variable named location to satisfy linting and reduce duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 880ffd72-1100-4d2e-b27f-8974ba693dab
📒 Files selected for processing (1)
tests/unit_tests/test_db.py
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit_tests/test_db.py (1)
2423-2492: ⚡ Quick winAdd regression tests for malformed
data_templatesinputs.These tests cover the happy path well; add a couple of negative cases (non-dict variants, unhashable selector values) to lock in resilience and prevent read-path regressions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit_tests/test_db.py` around lines 2423 - 2492, Add two regression tests to tests/unit_tests/test_db.py targeting apply_data_defaults: one where map_data["data_templates"] is a non-dict (e.g., a list or string) and assert apply_data_defaults(map_data, point) returns the original point (or is the same object) without raising; and one where a template selector is an unhashable value (e.g., using a list as a key under data_templates -> type_of_place) and assert apply_data_defaults(map_data, point) does not raise and leaves point unchanged. Use the existing test style and names similar to test_missing_data_templates_key_returns_point_unchanged and test_unmatched_template_key_ignored so these edge cases are covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit_tests/test_db.py`:
- Around line 2423-2492: Add two regression tests to tests/unit_tests/test_db.py
targeting apply_data_defaults: one where map_data["data_templates"] is a
non-dict (e.g., a list or string) and assert apply_data_defaults(map_data,
point) returns the original point (or is the same object) without raising; and
one where a template selector is an unhashable value (e.g., using a list as a
key under data_templates -> type_of_place) and assert
apply_data_defaults(map_data, point) does not raise and leaves point unchanged.
Use the existing test style and names similar to
test_missing_data_templates_key_returns_point_unchanged and
test_unmatched_template_key_ignored so these edge cases are covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55b33207-3e9a-45cf-ba5a-c5c830b10967
📒 Files selected for processing (3)
docs/quickstart.rstgoodmap/db.pytests/unit_tests/test_db.py
1 participant
Summary by CodeRabbit
New Features
Chores
Tests
Documentation