Raise `AttributeError` on attempts to access unset `oneof` fields #510

a-khabarov · 2023-07-11T13:43:45Z

This commit modifies Message.__getattribute__ to raise AttributeError whenever an attempt is made to access an unset oneof field. This provides several benefits over the current approach:

There is no longer any risk of betterproto users accidentally relying on values of unset fields.
Pattern matching with match/case on messages containing oneof groups is now supported. The following is now possible:
```
@dataclasses.dataclass(eq=False, repr=False)
class Test(betterproto.Message):
  x: int = betterproto.int32_field(1, group="g")
  y: str = betterproto.string_field(2, group="g")

match Test(y="text"):
  case Test(x=v):
    print("x", v)
  case Test(y=v):
    print("y", v) 
```
Before this commit the code above would output x 0 instead of y text, but now the output is y text as expected. The reason this works is because an AttributeError in a case pattern does not propagate and instead simply skips the case.
We now have a type-checkable way to deconstruct oneof. When running mypy for the snippet above v has type int in the first case and type str in the second case. For versions of Python that do not support match/case (before 3.10) it is now possbile to use try/except/else blocks to achieve the same result:
```
t = Test(y="text")
try:
  v0: int = t.x
except AttributeError:
  v1: str = t.y  # `oneof` contains `y`
else:
  pass  # `oneof` contains `x`
```

This is a breaking change. The previous behavior is still accessible via Message.__unsafe_get.

a-khabarov · 2023-07-11T13:59:08Z

I think this PR solves all of the problems mentioned in #358

src/betterproto/__init__.py

Gobot1234 · 2023-07-18T08:54:52Z

Thanks for the pr it looks good apart from the changes requiring you to access _Message__unsafe_get which should not be api that anyone should have to deal with. Unless this is just for testing purposes

a-khabarov · 2023-07-18T11:48:54Z

_Message__unsafe_get is just for testing/use within betterproto itself, not for the users of betterproto. I don't see any reason why the users of betterproto would want to call _Message__unsafe_get instead of just using ..

Gobot1234 · 2023-07-18T11:50:29Z

If it's just for testing like that can you just remove the method and use object.__getattribute__(message, name) in tests?

a-khabarov · 2023-07-18T11:54:59Z

Okay, I will remove it and update the tests to use object.__getattribute__(message, name).

a-khabarov · 2023-07-18T15:32:02Z

I've removed __unsafe_get and updated the tests to use object.__getattribute__(message, name).

src/betterproto/__init__.py

This commit modifies `Message.__getattribute__` to raise `AttributeError` whenever an attempt is made to access an unset `oneof` field. This provides several benefits over the current approach: * There is no longer any risk of `betterproto` users accidentally relying on values of unset fields. * Pattern matching with `match/case` on messages containing `oneof` groups is now supported. The following is now possible: ``` @dataclasses.dataclass(eq=False, repr=False) class Test(betterproto.Message): x: int = betterproto.int32_field(1, group="g") y: str = betterproto.string_field(2, group="g") match Test(y="text"): case Test(x=v): print("x", v) case Test(y=v): print("y", v) ``` Before this commit the code above would output `x 0` instead of `y text`, but now the output is `y text` as expected. The reason this works is because an `AttributeError` in a `case` pattern does not propagate and instead simply skips the `case`. * We now have a type-checkable way to deconstruct `oneof`. When running `mypy` for the snippet above `v` has type `int` in the first `case` and type `str` in the second `case`. For versions of Python that do not support `match/case` (before 3.10) it is now possbile to use `try/except/else` blocks to achieve the same result: ``` t = Test(y="text") try: v0: int = t.x except AttributeError: v1: str = t.y # `oneof` contains `y` else: pass # `oneof` contains `x` ``` This is a breaking change.

Gobot1234 · 2023-07-19T10:14:00Z

In future would you mind avoiding force pushing, it makes things harder to review and we squash the commits anyway

a-khabarov · 2023-07-19T10:44:32Z

For {group_current[group]} in attribute errors, do we also need !r? The output at the moment seems to be of the form AttributeError: 'g' is set to a, not 'b' (__repr__ not called for group_current[group]).

Gobot1234 · 2023-07-19T11:05:32Z

Would you mind also adding a small match test case for the feature you have shown in your OP?

a-khabarov · 2023-07-19T11:06:26Z

Okay, I will add it.

Gobot1234 · 2023-07-19T11:06:44Z

tests/test_features.py

    assert betterproto.which_one_of(foo, "group1")[0] == "baz"

-    foo.sub.val = 1


Is this a behaviour change?

This PR makes it impossible to use foo.sub.val = 1 when foo.sub is unset in the group.

I think this is a bit of a tradeoff. With this change the users of betterproto can no longer use the foo.sub.val = 1 syntax for fields that are unset in groups, but this also means that there is less risk of them accidentally changing which field is set in a group.

This is needed to allow formatting Python 3.10 pattern matching syntax.

The pattern matching syntax is only supported in Python 3.10+, so this commit adds it in a separate file to avoid syntax errors for older Python versions.

These is no `23.0.0` release.

Pass `--target-version py310` to allow pattern matching syntax.

a-khabarov · 2023-07-19T15:28:00Z

I've added a test for pattern matching. Sorry for all the commits, I should've installed pre-commit locally before pushing.

a-khabarov · 2023-07-21T12:02:45Z

@Gobot1234 Thank you! Would you mind merging this PR? (since I don't have write access)

MicaelJarniac · 2023-07-21T14:06:25Z

I think it'd be nice to have this reflected in the docs.

…nielgtaylor#510) # Conflicts: # poetry.lock # src/betterproto/__init__.py # src/betterproto/plugin/parser.py

…nielgtaylor#510) # Conflicts: # poetry.lock # src/betterproto/__init__.py # src/betterproto/plugin/parser.py # Conflicts: # poetry.lock # pyproject.toml

Removed the parts of the example that showed accessing an unset value, as it now raises an `AttributeError`, and added an example of the `match` way of accessing the attributes. Related to danielgtaylor#510 and danielgtaylor#358.

MicaelJarniac · 2024-02-22T20:47:14Z

I've opened a PR to change the README to reflect this:

Add one-of match example to README #558

Removed the parts of the example that showed accessing an unset value, as it now raises an `AttributeError`, and added an example of the `match` way of accessing the attributes. Related to #510 and #358.

a-khabarov force-pushed the safer-oneof-access branch from 2b1c650 to 105052c Compare July 11, 2023 13:47

a-khabarov mentioned this pull request Jul 11, 2023

Proposal to make oneof-deconstruction typecheckable #358

Closed

mattyclarkson reviewed Jul 12, 2023

View reviewed changes

src/betterproto/__init__.py Outdated Show resolved Hide resolved

src/betterproto/__init__.py Outdated Show resolved Hide resolved

a-khabarov force-pushed the safer-oneof-access branch from 105052c to c4b581a Compare July 12, 2023 15:15

a-khabarov force-pushed the safer-oneof-access branch from c4b581a to 774b3c2 Compare July 18, 2023 15:26

a-khabarov force-pushed the safer-oneof-access branch from 774b3c2 to ca47cc5 Compare July 18, 2023 15:45

Gobot1234 reviewed Jul 18, 2023

View reviewed changes

src/betterproto/__init__.py Outdated Show resolved Hide resolved

a-khabarov force-pushed the safer-oneof-access branch from ca47cc5 to 921a2b7 Compare July 19, 2023 10:07

Add a missing !r to AttributeError format strings

a07cde5

Gobot1234 reviewed Jul 19, 2023

View reviewed changes

a-khabarov added 6 commits July 19, 2023 15:59

Upgrade black

6719e42

This is needed to allow formatting Python 3.10 pattern matching syntax.

Test oneof pattern matching

764f926

The pattern matching syntax is only supported in Python 3.10+, so this commit adds it in a separate file to avoid syntax errors for older Python versions.

Fix pre-commit formatting and sys import location

784a3c2

Use black 23.1.0 instead of 23.0.0

708e46b

These is no `23.0.0` release.

Fix black pre-commit hook

c9340e6

Pass `--target-version py310` to allow pattern matching syntax.

Run black on all files

7506604

a-khabarov requested a review from Gobot1234 July 21, 2023 11:47

Gobot1234 approved these changes Jul 21, 2023

View reviewed changes

Gobot1234 merged commit 6faac1d into danielgtaylor:master Jul 21, 2023
17 checks passed

a-khabarov deleted the safer-oneof-access branch July 21, 2023 12:37

MicaelJarniac mentioned this pull request Feb 22, 2024

Add one-of match example to README #558

Merged

7 tasks

Gobot1234 mentioned this pull request Aug 11, 2024

betterproto.serialized_on_wire(..) on main is broken #592

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raise `AttributeError` on attempts to access unset `oneof` fields #510

Raise `AttributeError` on attempts to access unset `oneof` fields #510

a-khabarov commented Jul 11, 2023

a-khabarov commented Jul 11, 2023

Gobot1234 commented Jul 18, 2023 •

edited

Loading

a-khabarov commented Jul 18, 2023

Gobot1234 commented Jul 18, 2023 •

edited

Loading

a-khabarov commented Jul 18, 2023

a-khabarov commented Jul 18, 2023

Gobot1234 commented Jul 19, 2023

a-khabarov commented Jul 19, 2023 •

edited

Loading

Gobot1234 commented Jul 19, 2023

a-khabarov commented Jul 19, 2023

Gobot1234 Jul 19, 2023

a-khabarov Jul 19, 2023 •

edited

Loading

a-khabarov Jul 19, 2023

a-khabarov commented Jul 19, 2023

a-khabarov commented Jul 21, 2023

MicaelJarniac commented Jul 21, 2023

MicaelJarniac commented Feb 22, 2024

		assert betterproto.which_one_of(foo, "group1")[0] == "baz"

		foo.sub.val = 1

Raise AttributeError on attempts to access unset oneof fields #510

Raise AttributeError on attempts to access unset oneof fields #510

Conversation

a-khabarov commented Jul 11, 2023

a-khabarov commented Jul 11, 2023

Gobot1234 commented Jul 18, 2023 • edited Loading

a-khabarov commented Jul 18, 2023

Gobot1234 commented Jul 18, 2023 • edited Loading

a-khabarov commented Jul 18, 2023

a-khabarov commented Jul 18, 2023

Gobot1234 commented Jul 19, 2023

a-khabarov commented Jul 19, 2023 • edited Loading

Gobot1234 commented Jul 19, 2023

a-khabarov commented Jul 19, 2023

Gobot1234 Jul 19, 2023

Choose a reason for hiding this comment

a-khabarov Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

a-khabarov Jul 19, 2023

Choose a reason for hiding this comment

a-khabarov commented Jul 19, 2023

a-khabarov commented Jul 21, 2023

MicaelJarniac commented Jul 21, 2023

MicaelJarniac commented Feb 22, 2024

Raise `AttributeError` on attempts to access unset `oneof` fields #510

Raise `AttributeError` on attempts to access unset `oneof` fields #510

Gobot1234 commented Jul 18, 2023 •

edited

Loading

Gobot1234 commented Jul 18, 2023 •

edited

Loading

a-khabarov commented Jul 19, 2023 •

edited

Loading

a-khabarov Jul 19, 2023 •

edited

Loading