-
Notifications
You must be signed in to change notification settings - Fork 1
Description
Context
Follow-up from PR #21 code review. Three hardening opportunities that affect Cte.schema and related schema properties (Join.schema, Select.schema).
1. type(s)(...) assumes single-arg constructor for all Schema subtypes
Cte.schema, Join.schema (line 487), and Select.schema (line 585) all use type(s)(dict(...)) to reconstruct schemas. This breaks if a Schema subclass requires additional constructor args (e.g., db_name). The existing CaseAwareMapping.new() method was designed for this but is unused.
Consider using s.new(...) consistently, or documenting the single-arg constructor contract.
Files: data_diff/queries/ast_classes.py lines 487, 585, 647
2. Inner schema errors lose CTE context
When self.table.schema raises (e.g., from a nested Join without columns), the exception propagates without any CTE context. Wrapping with a chained exception would aid debugging:
try:
s = self.table.schema
except QueryBuilderError as exc:
raise QueryBuilderError(f"Failed to resolve schema for CTE: {exc}") from excFile: data_diff/queries/ast_classes.py, line 638
3. No runtime validation on params element types
params is annotated Sequence[str] | None but attrs doesn't enforce this at runtime. Passing params=[1, 2] produces confusing errors deep in CaseInsensitiveDict.__init__ (AttributeError: 'int' object has no attribute 'lower').
File: data_diff/queries/ast_classes.py, line 630