fix(entity_file): raise on unparseable ID_INTEGER values instead of silent fallthrough#74
fix(entity_file): raise on unparseable ID_INTEGER values instead of silent fallthrough#74gkorland wants to merge 2 commits into
ID_INTEGER values instead of silent fallthrough#74Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 46 minutes and 29 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
=======================================
Coverage 89.50% 89.50%
=======================================
Files 9 9
Lines 610 610
=======================================
Hits 546 546
Misses 64 64 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Copilot Code Review✅ No significant issues found. The PR correctly fixes inconsistent error handling for ID_INTEGER parsing failures. Error message is clearer, logic is sound, and backward compatibility is maintained (both old and new raise SchemaError). |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens schema-enforced ID handling in the bulk loader by ensuring that integer IDs (:ID(int) / Type.ID_INTEGER) fail fast with a clear SchemaError when their values are not parseable as integers, instead of relying on less-specific downstream behavior.
Changes:
- Raise a
SchemaErrorfor unparseableType.ID_INTEGERvalues (same behavior asType.LONG). - Improve the error message to include the offending value and a human-readable type name.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Both ID_INTEGER and LONG demand a parseable integer; falling | ||
| # through to the STRING branch would silently produce a | ||
| # type-confused ID that cannot be resolved by edge processing. |
There was a problem hiding this comment.
The new comment says this would "fall through to the STRING branch", but for Type.ID_INTEGER/Type.LONG this function doesn't enter the string branch; it will proceed through the remaining elifs and hit the final generic SchemaError. Please update the comment to reflect the actual control flow (and the specific failure mode you're preventing).
| # Both ID_INTEGER and LONG demand a parseable integer; falling | |
| # through to the STRING branch would silently produce a | |
| # type-confused ID that cannot be resolved by edge processing. | |
| # Both ID_INTEGER and LONG demand a parseable integer; without | |
| # this explicit error, control would continue past the remaining | |
| # elif branches and end in the final generic SchemaError instead | |
| # of reporting the specific integer/ID parse failure here. |
There was a problem hiding this comment.
Fixed — the comment now reads: 'without this explicit error, control would continue past the remaining elif branches and end in the final generic SchemaError instead of reporting the specific integer/ID parse failure here.' (commit d3d1d88)
| # Both ID_INTEGER and LONG demand a parseable integer; falling | ||
| # through to the STRING branch would silently produce a | ||
| # type-confused ID that cannot be resolved by edge processing. | ||
| type_name = "long" if prop_type == Type.LONG else "integer ID" | ||
| raise SchemaError(f"Could not parse '{prop_val}' as a {type_name}") |
There was a problem hiding this comment.
This change introduces a new, more specific failure mode/message for ID_INTEGER parsing. There are existing integration tests for integer IDs (success path), but no regression test that a non-integer :ID(...) value fails with this clear SchemaError message when --id-type integer is used. Adding a test would prevent this from regressing again.
There was a problem hiding this comment.
Done — added test/test_entity_file.py with four unit tests: valid and invalid paths for both Type.ID_INTEGER and Type.LONG. All four pass locally. (commit d3d1d88)
…ilent fallthrough Previously, when a column was declared as `:ID(int)` (ID_INTEGER) and the cell value could not be parsed as an integer, the code silently fell through every `elif`/`else` branch and returned `None`, since the `raise SchemaError` was guarded by `if prop_type == Type.LONG`. A `None` return is then passed back to `pack_props` and packed into the binary payload, producing a row that the server rejects with an opaque error and corrupting the ID-to-graph-node mapping that edge processing relies on. Treat ID_INTEGER and LONG identically: both must parse as an integer, otherwise raise `SchemaError` with a clear message naming the offending value and column type. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…GER parse failure - Update comment in typed_prop_to_binary to accurately describe the control flow: without the explicit raise, execution would continue past the remaining elif branches and end in the final generic SchemaError rather than reporting the specific integer/ID parse failure. - Add test/test_entity_file.py with unit tests covering both the valid and invalid paths for Type.ID_INTEGER and Type.LONG to prevent silent-fallthrough regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
eb68a3d to
d3d1d88
Compare
Agent Review SummaryPR: fix(entity_file): raise on unparseable Actions TakenRebased on Addressed 2 inline reviewer comments (Copilot PR Reviewer):
Test ResultsAll 8 unit tests pass ( Commit: d3d1d88 |
Summary
When a column declared as
:ID(int)(Type.ID_INTEGER) contained a value that could not be parsed as an integer,typed_prop_to_binarysilently fell through every branch and returnedNonebecause theraise SchemaErrorwas guarded byif prop_type == Type.LONG. TheNonewas packed into the binary payload, producing a row the server rejects with an opaque error and corrupting the ID-to-graph-node mapping edge processing depends on.Changes
falkordb_bulk_loader/entity_file.py: treatID_INTEGERandLONGidentically – both must parse as an integer, otherwise raiseSchemaErrorwith a clear message that names the offending value and the declared type.Testing
uv run flake8clean. The success path is unchanged; only the previously-silent error path is affected.Memory / Performance Impact
N/A.
Related Issues
From the comprehensive code-review report (BUG-2).