Raise AttributeError on attempts to access unset oneof fields
#510
Conversation
2b1c650 to
105052c
Compare
|
I think this PR solves all of the problems mentioned in #358 |
105052c to
c4b581a
Compare
|
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 |
|
_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 |
|
If it's just for testing like that can you just remove the method and use |
|
Okay, I will remove it and update the tests to use |
c4b581a to
774b3c2
Compare
|
I've removed |
774b3c2 to
ca47cc5
Compare
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.
ca47cc5 to
921a2b7
Compare
|
In future would you mind avoiding force pushing, it makes things harder to review and we squash the commits anyway |
|
For |
|
Would you mind also adding a small match test case for the feature you have shown in your OP? |
|
Okay, I will add it. |
| assert betterproto.which_one_of(foo, "group1")[0] == "baz" | ||
|
|
||
| foo.sub.val = 1 |
There was a problem hiding this comment.
Is this a behaviour change?
There was a problem hiding this comment.
This PR makes it impossible to use foo.sub.val = 1 when foo.sub is unset in the group.
There was a problem hiding this comment.
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.
|
I've added a test for pattern matching. Sorry for all the commits, I should've installed |
|
@Gobot1234 Thank you! Would you mind merging this PR? (since I don't have write access) |
|
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.
|
I've opened a PR to change the README to reflect this: |
This commit modifies
Message.__getattribute__to raiseAttributeErrorwhenever an attempt is made to access an unsetoneoffield. This provides several benefits over the current approach:There is no longer any risk of
betterprotousers accidentally relying on values of unset fields.Pattern matching with
match/caseon messages containingoneofgroups is now supported. The following is now possible:Before this commit the code above would output
x 0instead ofy text, but now the output isy textas expected. The reason this works is because anAttributeErrorin acasepattern does not propagate and instead simply skips thecase.We now have a type-checkable way to deconstruct
oneof. When runningmypyfor the snippet abovevhas typeintin the firstcaseand typestrin the secondcase. For versions of Python that do not supportmatch/case(before 3.10) it is now possbile to usetry/except/elseblocks to achieve the same result:This is a breaking change. The previous behavior is still accessible via
Message.__unsafe_get.