Skip to content

fix(serializer): handle cyclic dataclasses - #1838

Open
arpansahu wants to merge 1 commit into
langfuse:mainfrom
arpansahu:fix/cyclic-dataclass-serialization
Open

fix(serializer): handle cyclic dataclasses#1838
arpansahu wants to merge 1 commit into
langfuse:mainfrom
arpansahu:fix/cyclic-dataclass-serialization

Conversation

@arpansahu

@arpansahu arpansahu commented Aug 27, 2026

Copy link
Copy Markdown

Problem

Serializing a dataclass that contains a cycle currently falls through to the top-level fallback instead of preserving the serializable fields. dataclasses.asdict() recurses without using the serializer's cycle guard, so a cyclic dataclass becomes a quoted fallback string.

Reproducer

@dataclass
class Node:
    name: str
    next: "Node | None" = None

node1 = Node("first")
node2 = Node("second")
node1.next = node2
node2.next = node1

EventSerializer().encode(node1)
# before: '"\\"<not serializable object of type: Node>\\""'
# after:  '{"name":"first","next":{"name":"second","next":"Node"}}'

Fix

Serialize dataclass fields through EventSerializer.default() instead of asdict(), and reuse the existing seen guard so recursive dataclass references are represented by their type name like other cyclic objects.

Testing

  • python -m pytest tests\\unit\\test_serializer.py::test_circular_dataclass_reference -q failed before the fix and passed after.
  • python -m pytest tests\\unit\\test_serializer.py -k "not test_path" -q -> 30 passed, 1 deselected.
  • ruff format langfuse\\_utils\\serializer.py tests\\unit\\test_serializer.py --check -> 2 files already formatted.
  • ruff check langfuse\\_utils\\serializer.py tests\\unit\\test_serializer.py -> All checks passed.

Note: full offline tests\\unit baseline has pre-existing Windows/environment failures unrelated to this change (test_path, prompt subprocess missing opentelemetry, and prompt mock errors); the changed serializer subset passes.

Greptile Summary

The PR replaces dataclasses.asdict() with field-wise serialization guarded by the serializer’s existing cycle tracking and adds coverage for direct cyclic dataclass references.

  • Serializes each dataclass field through EventSerializer.default().
  • Emits a dataclass type name when a directly repeated instance is encountered.
  • Adds a unit test for a two-node direct-reference cycle.

Confidence Score: 4/5

The cyclic-dataclass fix should be completed for tuple, set, and frozenset fields before merging because those supported containers can still bypass cycle detection.

Field-wise serialization handles direct and recursively traversed container references, but raw elements returned from tuple, set, and frozenset conversion are encoded only after the enclosing dataclass has been removed from the cycle guard.

Files Needing Attention: langfuse/_utils/serializer.py

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Encode dataclass] --> B[Add object ID to seen]
  B --> C[Serialize each field]
  C --> D{Field container type}
  D -->|dict, list, Sequence| E[Recursively call default]
  D -->|tuple, set, frozenset| F[Return list with raw elements]
  E --> G[Repeated dataclass becomes type marker]
  F --> H[Remove enclosing dataclass from seen]
  H --> I[JSONEncoder encounters raw dataclass again]
  I --> A
Loading
Prompt To Fix All With AI
### Issue 1
langfuse/_utils/serializer.py:137-138
**Container cycles escape guard**

When a cyclic dataclass reference passes through a tuple, set, or frozenset field, that container returns the repeated dataclass as a raw element and the enclosing `finally` removes it from `seen` before `JSONEncoder` encounters it, causing serialization to recurse instead of emitting the type marker and preserving the structured fields.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Reviews (1): Last reviewed commit: "fix(serializer): handle cyclic dataclass..." | Re-trigger Greptile

Greptile also left 1 inline comment on this PR.

Context used:

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: arpan sahu <28574248+arpansahu@users.noreply.github.com>

@claude claude Bot 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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Comment on lines +137 to +138
return {
field.name: self.default(getattr(obj, field.name))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Container cycles escape guard

When a cyclic dataclass reference passes through a tuple, set, or frozenset field, that container returns the repeated dataclass as a raw element and the enclosing finally removes it from seen before JSONEncoder encounters it, causing serialization to recurse instead of emitting the type marker and preserving the structured fields.

Knowledge Base Used: Shared models and data serialization

Prompt To Fix With AI
This is a comment left during a code review.
Path: langfuse/_utils/serializer.py
Line: 137-138

Comment:
**Container cycles escape guard**

When a cyclic dataclass reference passes through a tuple, set, or frozenset field, that container returns the repeated dataclass as a raw element and the enclosing `finally` removes it from `seen` before `JSONEncoder` encounters it, causing serialization to recurse instead of emitting the type marker and preserving the structured fields.

**Knowledge Base Used:** [Shared models and data serialization](https://app.greptile.com/personal-org-4986/-/custom-context/knowledge-base/langfuse/langfuse-python/-/docs/shared-models-and-data-serialization.md)

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@arpansahu

Copy link
Copy Markdown
Author

Disclosure: this contribution was prepared with AI assistance (GitHub Copilot CLI), and
I am flagging that explicitly rather than leaving you to guess.

The defect, the fix and the regression test were verified locally: the new test fails on
unmodified upstream and passes with the change applied, and the surrounding suite is green.

If your project would prefer not to take AI-assisted contributions, or you would rather this
were reworked and resubmitted by hand, please just close it -- no objection at all, and
apologies for the noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants