Skip to content

fix(odin): Fix oversize metadata field truncation issue - #576

Merged
vikramlc-cognite merged 5 commits into
masterfrom
EDG-825-oversize-metadata-truncate-fix
Sep 11, 2026
Merged

vikramlc-cognite merged 5 commits into
masterfrom
EDG-825-oversize-metadata-truncate-fix

Conversation

@vikramlc-cognite

@vikramlc-cognite vikramlc-cognite commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that changes existing behavior)
  • Refactor (no functional change)
  • Documentation update
  • Chore / tooling / CI

What changed

  • cognite/extractorutils/unstable/core/base.py — _handle_custom_action success 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.
  • Same reordering applied to the parallel ActionError branch'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

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

  • The ActionError branch still has str(e) (itself unbounded) ahead of the reordered explanation — reordering doesn't fully protect against a pathologically long ActionError message on its own; that's a separate, pre-existing concern out of scope here.
  • Message wording change is user-facing (result_message surfaced 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 via git stash) — failures are pre-existing KeyError: '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

  • No config/migration changes; pure message-formatting fix. Revert the commit to roll back.

Checklist

  • Self-reviewed the diff
  • Tests added or updated
  • Docs updated (N/A — no user-facing docs describe this message format)
  • No secrets, credentials, or PII committed
  • Breaking changes called out above and communicated to affected teams (N/A — non-breaking)

@vikramlc-cognite

Copy link
Copy Markdown
Contributor Author

/gemini review

@vikramlc-cognite vikramlc-cognite self-assigned this Sep 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cognite/extractorutils/unstable/core/base.py Outdated

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread cognite/extractorutils/unstable/core/base.py Outdated
@codecov

codecov Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.09%. Comparing base (e5cf9ab) to head (8d1e65d).
⚠️ Report is 1 commits behind head on master.

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           
Files with missing lines Coverage Δ
cognite/extractorutils/unstable/core/base.py 86.90% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vikramlc-cognite
vikramlc-cognite marked this pull request as ready for review September 10, 2026 07:56
@vikramlc-cognite
vikramlc-cognite requested a review from a team as a code owner September 10, 2026 07:56
@vikramlc-cognite vikramlc-cognite added the waiting-for-risk-review Waiting for a member of the risk review team to take an action label Sep 10, 2026
@Khyat-Cognite Khyat-Cognite added risk-review-ongoing Risk review is in progress and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Sep 11, 2026
@Khyat-Cognite Khyat-Cognite self-assigned this Sep 11, 2026
Comment thread cognite/extractorutils/unstable/core/base.py Outdated
Comment thread tests/test_unstable/test_action_dispatch.py Outdated
@Khyat-Cognite

Copy link
Copy Markdown

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Khyat-Cognite Khyat-Cognite added the waiting-for-team Waiting for the submitter or reviewer of the PR to take an action label Sep 11, 2026
@vikramlc-cognite vikramlc-cognite added waiting-for-risk-review Waiting for a member of the risk review team to take an action and removed waiting-for-team Waiting for the submitter or reviewer of the PR to take an action labels Sep 11, 2026
@Khyat-Cognite

Copy link
Copy Markdown

🦄 -- LGTM ^^

@Khyat-Cognite Khyat-Cognite added waiting-for-team Waiting for the submitter or reviewer of the PR to take an action and removed waiting-for-risk-review Waiting for a member of the risk review team to take an action labels Sep 11, 2026
@vikramlc-cognite
vikramlc-cognite merged commit ba567ac into master Sep 11, 2026
8 checks passed
@vikramlc-cognite
vikramlc-cognite deleted the EDG-825-oversize-metadata-truncate-fix branch September 11, 2026 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk-review-ongoing Risk review is in progress waiting-for-team Waiting for the submitter or reviewer of the PR to take an action

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants