Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problem with tagged union example #533

Closed
RazerM opened this issue Apr 18, 2024 · 3 comments · Fixed by #534
Closed

Problem with tagged union example #533

RazerM opened this issue Apr 18, 2024 · 3 comments · Fixed by #534
Labels
regression This is an inintended change for the worse.
Milestone

Comments

@RazerM
Copy link

RazerM commented Apr 18, 2024

  • cattrs version: 23.2.3
  • Python version: 3.9-3.12
  • Operating System: macOS

Description

I was following the docs on tagged unions in https://github.com/python-attrs/cattrs/blob/main/docs/strategies.md

What I Did

I'm unable to get OtherAppleNotification to work:

from typing import Union

from attrs import define
from cattrs import Converter
from cattrs.strategies import configure_tagged_union


@define
class Refund:
    originalTransactionId: str


@define
class OtherAppleNotification:
    notificationType: str


AppleNotification = Union[Refund, OtherAppleNotification]

c = Converter()
configure_tagged_union(
    AppleNotification,
    c,
    tag_name="notificationType",
    tag_generator={Refund: "REFUND"}.get,
    default=OtherAppleNotification,
)

payload = {"notificationType": "REFUND", "originalTransactionId": "1"}
notification = c.structure(payload, AppleNotification)
print(f"Structured {payload} as {notification}")

# All code above is from example

other = OtherAppleNotification(notificationType="OTHER")
payload = c.unstructure(other, AppleNotification)
print(f"Unstructured {other} as {payload}")

payload = {"notificationType": "OTHER"}
notification = c.structure(payload, AppleNotification)) # this line raises
print(f"Structured {payload} as {notification}")

I get the following output:

(1) This is given in the example and works fine:

Structured {'notificationType': 'REFUND', 'originalTransactionId': '1'} as Refund(originalTransactionId='1')

(2) Given the example I expected notificationType to be preserved, not None:

Unstructured OtherAppleNotification(notificationType='OTHER') as {'notificationType': None}

(3) I'm not able to structure a payload as OtherAppleNotification via the AppleNotification union:

  + Exception Group Traceback (most recent call last):
  |   File "scratch_175.py", line 40, in <module>
  |     print(c.structure(payload, AppleNotification))
  |   File "python3.9/site-packages/cattrs/converters.py", line 332, in structure
  |     return self._structure_func.dispatch(cl)(obj, cl)
  |   File "python3.9/site-packages/cattrs/converters.py", line 632, in _structure_union
  |     return handler(obj, union)
  |   File "python3.9/site-packages/cattrs/strategies/_unions.py", line 106, in structure_tagged_union
  |     return _tag_to_hook[val.pop(_tag_name)](val)
  |   File "python3.9/site-packages/cattrs/strategies/_unions.py", line 71, in structure_default
  |     return _h(val, _cl)
  |   File "<cattrs generated structure __main__.OtherAppleNotification>", line 9, in structure_OtherAppleNotification
  |     if errors: raise __c_cve('While structuring ' + 'OtherAppleNotification', errors, __cl)
  | cattrs.errors.ClassValidationError: While structuring OtherAppleNotification (1 sub-exception)
  +-+---------------- 1 ----------------
    | Traceback (most recent call last):
    |   File "<cattrs generated structure __main__.OtherAppleNotification>", line 5, in structure_OtherAppleNotification
    |     res['notificationType'] = __c_structure_notificationType(o['notificationType'])
    | KeyError: 'notificationType'
    | Structuring class OtherAppleNotification @ attribute notificationType
    +------------------------------------
@Tinche
Copy link
Member

Tinche commented Apr 18, 2024

Hm, confirming. Looks like this was broken in #443.

That PR pops out the tag out of the payload to make the strategy work with forbid_extra_keys. But it does feel useful to be able to have the tag.

@Tinche Tinche added this to the 24.1 milestone Apr 18, 2024
@Tinche Tinche added the regression This is an inintended change for the worse. label Apr 18, 2024
@Tinche Tinche linked a pull request Apr 18, 2024 that will close this issue
@Tinche
Copy link
Member

Tinche commented Apr 18, 2024

Thanks, got it fixed. I also turned the example into a doctest so we don't get into the same situation in the future.

@RazerM
Copy link
Author

RazerM commented Apr 18, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression This is an inintended change for the worse.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants