Visitar URL original
Fix dataclass init=False field deserialization by AmitKulkarni23 · Pull Request #1942 · temporalio/sdk-python · GitHub
Skip to content

Fix dataclass init=False field deserialization - #1942

Merged
tconley1428 merged 3 commits into
temporalio:mainfrom
AmitKulkarni23:fix/dataclass-init-false-deserialization
Oct 9, 2026
Merged

Fix dataclass init=False field deserialization#1942
tconley1428 merged 3 commits into
temporalio:mainfrom
AmitKulkarni23:fix/dataclass-init-false-deserialization

Conversation

@AmitKulkarni23

@AmitKulkarni23 AmitKulkarni23 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

What was changed

Skip dataclass fields with init=False when deserializing payloads back into dataclass instances in value_to_type. Previously, all fields were passed as constructor keyword arguments, causing a TypeError for fields excluded from __init__.

Fixes #1934

Why

When a dataclass has a field like:

@dataclasses.dataclass(frozen=True)
class MyModel:
    name: str = dataclasses.field(default="my-model", init=False)

Serialization works fine (dataclasses.asdict() includes all fields), but deserialization called MyModel(name="my-model") which raises TypeError because name is not a constructor parameter.

How

Added a check in temporalio/converter/_payload_converter.py to skip fields where field.init is False during dataclass construction. These fields get their values from defaults or __post_init__, as Python intends.

Test plan

  • Added InitFalseDataClass (frozen dataclass with init=False field) and a round-trip test in test_json_type_hints
  • All 40 converter tests pass
  • Added changelog fragment

Co-authored-by: Claude noreply@anthropic.com

@AmitKulkarni23
AmitKulkarni23 requested a review from a team as a code owner October 7, 2026 04:03
@CLAassistant

CLAassistant commented Oct 7, 2026 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@AmitKulkarni23
Fix dataclass init=False field deserialization. Skip non-init fields …
5f3ed39 
…when constructing dataclass instances from payloads.
@AmitKulkarni23
AmitKulkarni23 force-pushed the fix/dataclass-init-false-deserialization branch from 0530dc5 to 5f3ed39 Compare October 7, 2026 04:05
@AmitKulkarni23 AmitKulkarni23 mentioned this pull request Oct 7, 2026
Closed
tomasfarias
tomasfarias reviewed Oct 7, 2026
View reviewed changes

@tomasfarias tomasfarias left a comment •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not a maintainer here so the final call (or any call for that matter) is not mine. But I am the person who wrote the bug report, so I have some comments to make as this PR does not fully address my report.

  1. This PR misses the problem of validating union types, like I explained already:

One thing to keep in mind is that init=False fields should still be passed to value_to_type to ensure they are used to validate the type to use in the event a union is used, just not passed to the constructor on the final line of the dataclasses block.

By skipping these fields at the beginning of the loop, they are not used to validate which type of a union is the correct one, so one could end up with the wrong type when decoding if non init=False fields match between multiple types.

  1. Additionally, a decision has to be made regarding preserving state when decoding. Something that was also mentioned in my bug report:

and (optionally) assign the value after construction (although this would require checking that the classes are not frozen=True, or deliberately bypassing frozen=True by using setattr, which I would say goes too far).

In short, two questions need to be answered:

  • Do we preserve state of init=False fields by assigning them after the constructor call? This PR implicitly answers "No" but doesn't provide any rationale.
  • If yes, do we "override" the restriction imposed by a frozen=True dataclass? Again, this PR just ignores this question.

I appreciate the work, as I really just want to see this bug fixed (assuming it is accepted as a bug by maintainers), but this PR doesn't address my bug report so I felt compelled to comment. Ultimately, I am not a maintainer here, so just leaving a comment with my thoughts.

@tconley1428

tconley1428 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

I would agree with Tomas, I think this previous PR was closer, though I think it probably still needs test coverage for unions. https://github.com/temporalio/sdk-python/pull/1920/changes

@tconley1428 tconley1428 self-assigned this Oct 7, 2026
@AmitKulkarni23

AmitKulkarni23 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @tomasfarias and @tconley1428. I've reworked this.

  • My first version skipped init=False fields entirely. That stopped the TypeError, but serialized values were lost on decode, and the fields were never type-checked, so a union could pick the wrong class.

  • It now follows Restore init=False dataclass fields when converting from JSON #1920: every field is converted, the init=False ones are set after construction (including on frozen dataclasses), and there are tests for round trip, frozen, and union matching.

  • On pydantic: 2.13.4 (TypeAdapter and Temporal's pydantic_data_converter) drops init=False values on all dataclasses, not just frozen ones, and gets the union case wrong too. This PR is stricter, and I'm happy to switch to matching pydantic if you'd prefer.

tomasfarias
tomasfarias approved these changes Oct 9, 2026
View reviewed changes

@tomasfarias tomasfarias left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me! I think it's now covering well the issues with my bug report.

I saw the tests, and they look enough, but I am also not as familiar with this repo to make the final call. From my side I am approving. Thanks to all involved in getting this through.

@tconley1428
tconley1428 merged commit fded39f into temporalio:main Oct 9, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tconley1428 tconley1428 tconley1428 approved these changes
+1 more reviewer
@tomasfarias tomasfarias tomasfarias approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@tconley1428 tconley1428

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Dataclass fields with init=False break payload round tripping

4 participants

@AmitKulkarni23 @CLAassistant @tconley1428 @tomasfarias