dbt-materialize: report clear errors on the adapter's error paths - #38070
Conversation
1759db8 to
dd97151
Compare
jubrad
left a comment
There was a problem hiding this comment.
Six real fixes, all of them doing what the changelog says; I verified the backslash escaping against pgwire's own split_options and the new drop_relation error against what the relation cache can actually produce, and both hold up.
Summary
2 considerations
written by claude on behalf of @jubrad
| f" Got: {raw_index}\n" | ||
| ' Expected a dictionary with at minimum a "columns" key' | ||
| ) | ||
| raise IndexConfigNotDictError(raw_index) |
There was a problem hiding this comment.
consider — IndexConfigNotDictError looks unreachable, and the tests agree with that reading: cls.validate reports non-dict input as a ValidationError, so parse("a") lands in IndexConfigError, and test_not_dict_error_message has to construct the exception directly to exercise its message.
Since the class exists only to be caught by a branch nothing takes, consider dropping the except TypeError arm and the class with it, and keeping just the ValidationError path. That also removes a message users can never see, which is otherwise the kind of thing that rots. (RefreshIntervalConfigNotDictError is presumably in the same boat, but that one is pre-existing and out of scope here.)
written by claude on behalf of @jubrad
There was a problem hiding this comment.
Done, dropped the except TypeError arm, the class, and the test that had to construct it directly.
| assert "options" in credentials._connection_keys() | ||
|
|
||
|
|
||
| def test_backslashes_are_escaped(): |
There was a problem hiding this comment.
consider — These two assertions restate the implementation rather than test it: they check that \ becomes \\, which is exactly the line of code above them. What makes the fix correct is the parser on the other end, so the escaping order is only really proven by a round trip.
tests/adapter/test_connection_options.py already has TestConnectionOptionsOverrideEscapeSpaces doing precisely this for spaces. A sibling class with application_name: "app\\with\\backslashes" asserting current_setting('application_name') comes back unchanged would catch a future regression in the escaping order, which these unit tests would happily continue to pass through.
written by claude on behalf of @jubrad
There was a problem hiding this comment.
Fair point, replaced them with TestConnectionOptionsOverrideEscapeBackslashes in test_connection_options.py, mirroring the spaces sibling. The value is app\with\ backslashes, so it includes a backslash directly before a space, which pins the escaping order through a real round trip rather than restating the replace calls. Kept test_non_string_values_are_stringified since that one covers the AttributeError fix rather than the escaping line.
dd97151 to
d92738a
Compare
A handful of error paths in the adapter did not do what they looked like they did, so users hit confusing failures instead of the messages we wrote for them. This fixes the index config parser, the incremental and listagg explanations, the branch in rename_relation that read an undefined variable, the missing fallback in drop_relation, backslash and non-string handling in connection options, and the version check against a server that is not Materialize.
Test plan: added
test_relation_macros.pyfor the index config error, renaming a view and dropping an unsupported relation type, plus a case intest_incremental.pythat checks the explanation is what users actually see.