Skip to content

Narrow exception catch in _derive_from_expr to stop masking errors#186

Merged
amc-corey-cox merged 6 commits intomainfrom
fix-expr-exception-185
Apr 1, 2026
Merged

Narrow exception catch in _derive_from_expr to stop masking errors#186
amc-corey-cox merged 6 commits intomainfrom
fix-expr-exception-185

Conversation

@amc-corey-cox
Copy link
Copy Markdown
Contributor

@amc-corey-cox amc-corey-cox commented Apr 1, 2026

Summary

Closes #185

Fixes the broad except Exception in _derive_from_expr that masked real runtime errors as "Expression not in safe subset." Also adds arithmetic coercion for numeric strings and an is_numeric() expression function so curators can handle messy tabular data.

Before

Expression not in safe subset: {phv00112691} / 100.0 * {phv00112686}

After

could not convert string to float: 'A'; class_derivation=Quantity (from pht004041); slot_derivation=value_decimal (from phv00191041)

Changes

Exception handling

  • Restricted mode: Only InvalidExpression (simpleeval safety violations) is caught. Real runtime errors (TypeError, ValueError, ZeroDivisionError) propagate as TransformationError with full context (class, slot, populated_from, source row).
  • Unrestricted mode: InvalidExpression, TypeError, and ValueError fall through to the asteval fallback (preserving existing behavior for multi-line expressions).

Arithmetic coercion

  • Arithmetic operators now coerce numeric strings to float: "100" / "50"2.0
  • Non-coercible operands return None with a warning: "abc" * 10None (logged)
  • This is necessary because case() evaluates eagerly — without it, curators can't guard expressions with is_numeric() since the guarded branch would crash before case() checks the condition

New expression function

  • is_numeric(x): Returns True if x can be converted to float, False otherwise. Enables guard patterns:
    expr: "case((is_numeric(x), x * 2.54), (True, None))"

Shared helper

  • _try_numeric(value): Extracted as a top-level helper for numeric coercion with bool exclusion. Used by both _is_numeric and _null_propagating.

Test plan

  • 553 tests pass, 0 failures
  • Restricted mode: real errors propagate with actual messages
  • Unrestricted mode: asteval fallback works for complex expressions
  • Arithmetic coerces numeric strings, returns None for non-numeric with warning
  • is_numeric() guard pattern works with case()
  • Division by zero still raises (not swallowed)
  • Range override tests updated: null → None (null propagation), malformed → raises (restricted mode)

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings April 1, 2026 19:26
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR narrows exception handling in ObjectTransformer._derive_from_expr so restricted evaluation no longer masks genuine runtime/data errors behind a generic “safe subset” message, improving curator-facing diagnostics while preserving the unrestricted asteval fallback behavior.

Changes:

  • Update _derive_from_expr to only fall back to asteval when unrestricted_eval=True, while letting real errors propagate in restricted mode to be wrapped as TransformationError with context.
  • Remove the misleading “Expression not in safe subset” wrapper behavior in restricted mode.
  • Adjust the restricted-mode test to use an expression that is reliably rejected by simpleeval and assert on the real underlying error message.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/linkml_map/transformer/object_transformer.py Restricts exception catching in expression derivation so restricted mode preserves real error messages and unrestricted mode continues to fall back to asteval.
tests/test_transformer/test_object_transformer.py Updates restricted-mode test to reflect new error propagation behavior and to use a consistently-rejected expression.

@amc-corey-cox amc-corey-cox force-pushed the fix-expr-exception-185 branch 2 times, most recently from 78a7791 to 9d23bc7 Compare April 1, 2026 20:06
Copilot AI review requested due to automatic review settings April 1, 2026 20:06
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

…tion

Stop masking real errors as "Expression not in safe subset" — only catch
InvalidExpression from simpleeval. Real errors propagate as proper
TransformationErrors with full context.

Arithmetic operators now coerce numeric strings to float and return None
(with a warning) for non-coercible values. This enables case() guards
with is_numeric() since case() evaluates eagerly:

  case((is_numeric(x), x * 2.54), (True, None))

Closes #185

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amc-corey-cox amc-corey-cox force-pushed the fix-expr-exception-185 branch from 9d23bc7 to e7b201f Compare April 1, 2026 20:19
- is_numeric(True) now returns False (consistent with other coercion helpers)
- Add ValueError to unrestricted eval fallback catch
- Clarify _null_propagating docstring re: + on strings
- Loosen lambda test match to case-insensitive "lambda"
- Fix range_override tests to use restricted mode for error assertions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 1, 2026 20:28
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

amc-corey-cox and others added 2 commits April 1, 2026 15:35
Reduces duplication: _try_numeric handles coercion to numeric with
bool exclusion, _is_numeric wraps it as a boolean check. Both
_null_propagating and _is_numeric now use the same coercion logic.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 1, 2026 20:37
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@amc-corey-cox amc-corey-cox merged commit 9c1a0e3 into main Apr 1, 2026
13 checks passed
@amc-corey-cox amc-corey-cox deleted the fix-expr-exception-185 branch April 1, 2026 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Overly broad exception catch in _derive_from_expr masks real errors

2 participants