fix(mappers): Support simpleeval 1.0.5+ by subclassing simpleeval.EvalWithCompoundTypes#3595
Conversation
Reviewer's GuideIntroduces a dedicated _MapperEval subclass of simpleeval.EvalWithCompoundTypes to restore compatibility with simpleeval 1.0.5+ while preserving mapper safety guarantees, replaces direct simpleeval usage in mapper evaluation paths with this subclass, wraps exposed datetime/json modules with simpleeval.ModuleWrapper, and updates the simpleeval dependency to a fixed 1.0.7 version. Sequence diagram for mapper record transformation using _MapperEvalsequenceDiagram
actor MapperUser
participant CustomStreamMap
participant _MapperEval
MapperUser->>CustomStreamMap: transform(record)
activate CustomStreamMap
CustomStreamMap->>CustomStreamMap: _eval(expr, record)
CustomStreamMap->>_MapperEval: eval(expr)
activate _MapperEval
_MapperEval->>_MapperEval: _eval(ast_root)
_MapperEval->>_MapperEval: _eval_name(Name)
_MapperEval-->>CustomStreamMap: evaluated_value
deactivate _MapperEval
CustomStreamMap-->>MapperUser: transformed_record
deactivate CustomStreamMap
Sequence diagram for stream selection evaluation via _eval_streamsequenceDiagram
participant Caller
participant _eval_stream
participant _MapperEval
Caller->>_eval_stream: _eval_stream(expr, stream_name)
activate _eval_stream
_eval_stream->>_eval_stream: build names dict
_eval_stream->>_MapperEval: _MapperEval(names=names)
activate _MapperEval
_MapperEval-->>_eval_stream: expr_evaluator
deactivate _MapperEval
_eval_stream->>_MapperEval: eval(expr)
activate _MapperEval
_MapperEval->>_MapperEval: _eval(ast_root)
_MapperEval-->>_eval_stream: result
deactivate _MapperEval
_eval_stream-->>Caller: result
deactivate _eval_stream
Class diagram for mapper evaluation changes using _MapperEvalclassDiagram
direction LR
class EvalWithCompoundTypes {
<<external>>
+dict names
+dict functions
+dict nodes
+eval(expr)
+_eval(node)
+_eval_name(node)
+_eval_attribute(node)
+_eval_call(node)
+_check_disallowed_items(item)
}
class _MapperEval {
<<internal>>
+dict names
+dict functions
+dict nodes
+_MapperEval(names, functions)
+_eval(node)
+_eval_name(node)
+_check_disallowed_items(item)
}
class CustomStreamMap {
+dict functions
+simpleeval.EvalWithCompoundTypes expr_evaluator
+CustomStreamMap()
+transform(record)
+_init_functions_and_schema(stream_map)
+_init_faker_instance()
+_eval(expr, record)
}
class simpleeval_ModuleWrapper {
<<external>>
+simpleeval_ModuleWrapper(module)
}
class datetime_module {
<<module>>
}
class json_module {
<<module>>
}
EvalWithCompoundTypes <|-- _MapperEval
CustomStreamMap --> _MapperEval : uses_for_expr_evaluator
CustomStreamMap --> simpleeval_ModuleWrapper : wraps_modules
simpleeval_ModuleWrapper --> datetime_module : wraps
simpleeval_ModuleWrapper --> json_module : wraps
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3595 +/- ##
=======================================
Coverage 93.73% 93.73%
=======================================
Files 73 73
Lines 5890 5890
Branches 723 723
=======================================
Hits 5521 5521
Misses 274 274
Partials 95 95
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…EvalWithCompoundTypes` Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
…ion boundary Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
4a5619c to
93a1cdf
Compare
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
simpleeval 1.0.5+ by subclassing simpleeval.EvalWithCompoundTypessimpleeval 1.0.5+ by subclassing simpleeval.EvalWithCompoundTypes
simpleeval 1.0.5+ by subclassing simpleeval.EvalWithCompoundTypessimpleeval 1.0.5+ by subclassing simpleeval.EvalWithCompoundTypes
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
This reverts commit c25109b.
Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
_MapperEvalsubclass currently reimplements_evaland_eval_namevery closely toEvalWithCompoundTypes; consider whether overriding only_check_disallowed_items(and relying on the base class for the rest) is sufficient to reduce maintenance risk when simpleeval internals change. - The dependency constraint for
simpleevalis pinned to==1.0.7while the PR description mentions supporting 1.0.5+; consider loosening this to a compatible range (e.g.>=1.0.5,<2.0) or updating the description to reflect the tighter pin.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_MapperEval` subclass currently reimplements `_eval` and `_eval_name` very closely to `EvalWithCompoundTypes`; consider whether overriding only `_check_disallowed_items` (and relying on the base class for the rest) is sufficient to reduce maintenance risk when simpleeval internals change.
- The dependency constraint for `simpleeval` is pinned to `==1.0.7` while the PR description mentions supporting 1.0.5+; consider loosening this to a compatible range (e.g. `>=1.0.5,<2.0`) or updating the description to reflect the tighter pin.
## Individual Comments
### Comment 1
<location path="singer_sdk/mapper.py" line_range="292-294" />
<code_context>
+ else:
+ return val
+
+ if callable(self.names):
+ try:
+ val = self.names(node) # ty:ignore[call-top-callable]
+ self._check_disallowed_items(val)
+ except simpleeval.NameNotDefined:
</code_context>
<issue_to_address>
**issue (bug_risk):** Passing the full `ast.Name` node into `self.names` deviates from simpleeval’s expected callable contract and may break existing callables.
In simpleeval, a callable `names` is invoked as `self.names(node.id)` (a `str`), not with the AST node. Switching to `self.names(node)` changes the callable’s public contract and can break existing implementations that expect a string per the library’s documentation. Unless you explicitly need the full node, please continue passing `node.id` instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…EvalWithCompoundTypes` (#3595) ## Related - Closes #3561 - Re-implements upstream danthedeckie/simpleeval#181 ## Summary by Sourcery Support newer simpleeval versions in mapper expression evaluation while preserving safety guarantees for stream map expressions. Bug Fixes: - Restore compatibility with simpleeval 1.0.5+ by customizing evaluation to bypass redundant disallowed-item checks that conflict with mapper usage. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and safety constraints. - Wrap datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Relax the simpleeval dependency to require version 1.0.5 or later in project configuration. ## Summary by Sourcery Restore compatibility of mapper expression evaluation with newer simpleeval versions while maintaining existing safety guarantees. Bug Fixes: - Fix incompatibility with simpleeval 1.0.5+ by using a custom evaluator that bypasses redundant disallowed-item checks conflicting with mapper use cases. Enhancements: - Introduce a dedicated _MapperEval subclass to centralize mapper-specific simpleeval behavior and security constraints. - Wrap the datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval's security model. Build: - Update the simpleeval dependency constraint to pin to version 1.0.7. --------- Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
…EvalWithCompoundTypes` (#3601) ## Related - #3561 - #3595 - Re-implements upstream danthedeckie/simpleeval#181 ## Summary by Sourcery Update stream mapper expression evaluation to be compatible with simpleeval 1.0.5+ while preserving the mapper’s existing safety guarantees. New Features: - Introduce a dedicated mapper expression evaluator subclass to control how AST nodes, names, and disallowed items are handled during evaluation. Bug Fixes: - Fix incompatibility with simpleeval 1.0.5+ by bypassing its new redundant container safety scan in a controlled way. Enhancements: - Wrap datetime and json modules with simpleeval.ModuleWrapper when exposing them as mapper functions to align with simpleeval’s safety model. Build: - Pin simpleeval to version 1.0.7 in project dependencies. Signed-off-by: Edgar Ramírez Mondragón <edgarrm358@gmail.com>
Related
simpleeval1.0.5+ by implementingModuleWrapperfor modules exposed to stream maps expressions #3561Summary by Sourcery
Support newer simpleeval versions in mapper expression evaluation while preserving safety guarantees for stream map expressions.
Bug Fixes:
Enhancements:
Build:
Summary by Sourcery
Restore compatibility of mapper expression evaluation with newer simpleeval versions while maintaining existing safety guarantees.
Bug Fixes:
Enhancements:
Build: