fix(firestore): Simplify pipeline aliases#16651
Conversation
daniel-sanche
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents calling expression methods on aliased expressions and prohibits chaining multiple aliases by introducing a _supports_expr_methods flag and adding validation logic. The test suite was updated to cover these cases, though the feedback recommends using more specific exception types in pytest.raises instead of a broad Exception to ensure tests do not inadvertently pass on unrelated errors.
| # check if server responds as expected | ||
| with pytest.raises(GoogleAPIError) as err: | ||
|
|
||
| with pytest.raises(Exception) as err: |
There was a problem hiding this comment.
Catching a base Exception is too broad and can mask unrelated bugs or unexpected errors (e.g., NameError, AttributeError, or malformed test data) that might occur during pipeline parsing or execution. Since the expected errors are either client-side validation errors (TypeError) or server-side errors (GoogleAPIError), it is better to catch only those specific types.
| with pytest.raises(Exception) as err: | |
| with pytest.raises((GoogleAPIError, TypeError)) as err: |
| pipeline = parse_pipeline(async_client, test_dict["pipeline"]) | ||
| # check if server responds as expected | ||
| with pytest.raises(GoogleAPIError) as err: | ||
| with pytest.raises(Exception) as err: |
There was a problem hiding this comment.
Catching a base Exception is too broad. It is recommended to use a more specific tuple of expected exceptions, such as (GoogleAPIError, TypeError), to ensure that the test only passes for the intended failure modes and doesn't swallow unrelated issues.
| with pytest.raises(Exception) as err: | |
| with pytest.raises((GoogleAPIError, TypeError)) as err: |
1 participant
Currently, AliasedExpressions are treated like regular expressions. You can execute additional expressions off of them (
a.as_("number").add(5)), or chain them (a.as_("first").as_("second")). But the backend doesn't actually support aliases being used in this wayThis PR raises an exception is an alias is used in a context it doesn't support
Go version: googleapis/google-cloud-go#14440