fix(odin): Fix oversize metadata field truncation issue - #576
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the result messages when custom action metadata fields exceed the byte limit by including the count of oversized fields and appending the list of dropped fields to the end of the message. It also adds corresponding regression tests. The reviewer suggested reordering the error message format to ensure the metadata dropped warning survives truncation even if the exception message is extremely long.
There was a problem hiding this comment.
Code Review
This pull request improves the reporting of oversized metadata fields in custom actions by including the count of oversized fields and appending the list of fields to the end of the message. It also adds corresponding regression tests. The reviewer pointed out that if the exception message is extremely long, the explanation of dropped metadata fields could be lost during truncation, and suggested truncating the exception message to a safe limit first.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #576 +/- ##
=======================================
Coverage 84.09% 84.09%
=======================================
Files 46 46
Lines 4709 4709
=======================================
Hits 3960 3960
Misses 749 749
🚀 New features to boost your workflow:
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request improves the formatting of error and result messages when custom action metadata fields exceed the size limit. By placing the count of oversized fields and the explanation of the limit violation before the list of dropped fields, the explanation is guaranteed to survive message truncation. Additionally, regression tests have been added to verify that key terms like 'exceeded' and 'dropped' remain in the truncated messages. No review comments were provided, so there is no additional feedback.
|
🦄 -- LGTM ^^ |
Summary
Fixes truncated custom-action result messages so the explanation of why metadata fields were dropped always survives truncation, instead of being pushed off the end by an unbounded list of field names.
Type of change
What changed
cognite/extractorutils/unstable/core/base.py—_handle_custom_actionsuccess path: reordered the oversized-metadata message so the fixed explanation ("N metadata field(s) exceeded the byte limit and were dropped") comes before the unbounded field-name list, and reports a count instead of only names.ActionErrorbranch's message.tests/test_unstable/test_action_dispatch.py— added regression assertions to the many-fields test confirming "exceeded"/"dropped" survive truncation.Why it changed
discussion_r3911633979)truncate_message()truncates from the end at 1000 chars. With the old ordering, a large enough number of oversized fields (e.g. 45, as in the existing regression test) pushed the trailing explanation entirely off the message, leaving only a cut-off list of field names with no indication of what happened or why.What to focus on during review
ActionErrorbranch still hasstr(e)(itself unbounded) ahead of the reordered explanation — reordering doesn't fully protect against a pathologically longActionErrormessage on its own; that's a separate, pre-existing concern out of scope here.result_messagesurfaced to Odin/callers) — confirm no downstream parsing depends on the old phrasing.Test evidence
python -m pytest tests/test_unstable/test_action_dispatch.py -q→ 37 passed.python -m pytest tests/test_unstable/ -q→ 248 passed / 1 failed / 48 errors, identical to the pre-change baseline (verified viagit stash) — failures are pre-existingKeyError: 'COGNITE...'env-fixture issues unrelated to this change.Risks and unknowns
ActionError.__str__()can itself exceed 1000 chars, in which case the explanation still gets truncated away regardless of ordering — not addressed here, flagged as a possible follow-up.Rollout and rollback
Checklist