PLACEHOLDER test sometimes fails when using pydantic

AdrienVannson commented 2 months ago

Summary

PLACEHOLDER test sometimes fails when using pydantic

Reproduction Steps

Compile the following messages with pydantic:

message A {
    B v = 1;
}

message B {
    A v = 1;
}

In Python, import the messages and do print(A()).

Expected Results

I would have expected this instruction to print only A().

Actual Results

This prints: A(v=<object object at 0x7f081c1f1a10>)

The problems comes from the test if ... is PLACEHOLDER in the __repr__ function: the field is considered different from PLACEHOLDER, even if it is an object() as well. Similar errors can occur in other contexts where comparing the field to PLACEHOLDER is needed.

After some time, I think I finally understood the reason: pydantic dataclasses behave differently from python dataclasses when given a mutable default value: pydantic automatically detects this and performs a deep copy. Thus, the value put in the field is not the PLACEHOLDER but a copy of this instance. This makes all the tests if field is PLACEHOLDER fail.

In my example, I used nested messages since it was the easiest way that I found to reproduce the bug, but I think I came across it in other circumstances.

This behavior of pydantic has been discussed here: https://github.com/pydantic/pydantic/issues/3728 I think a potential solution could be to set PLACEHOLDER to an immutable value, as suggested by the last message of the conversation. It solves the problem in my example, but I need to further test this solution. I can send a pull-request soon if you agree with this solution.

System Information

libprotoc 3.12.4 Python 3.12.5 Name: betterproto Version: 2.0.0b7 Summary: A better Protobuf / gRPC generator & library Home-page: https://github.com/danielgtaylor/python-betterproto Author: Daniel G. Taylor Author-email: danielgtaylor@gmail.com License: MIT Location: XXX Requires: grpclib, python-dateutil, typing-extensions Required-by: pygrpc-poc

Checklist

[X] I have searched the issues for duplicates.
[X] I have shown the entire traceback, if possible.
[X] I have verified this issue occurs on the latest prelease of betterproto which can be installed using pip install -U --pre betterproto, if possible.

Gobot1234 commented 2 months ago

I would have thought object would have counted though honestly given the constraints of we can only have valid protobuf types as values we could always just use something like EllipsisType or NoneType

AdrienVannson commented 2 months ago

I submitted a PR: https://github.com/danielgtaylor/python-betterproto/pull/611

Using ellipsis did not work since Pydantic handles it differently, and I did not want to use None to avoid interfering with optional fields. The types that are not deep copied by pydantic are listed in IMMUTABLE_NON_COLLECTIONS_TYPES ( https://github.com/pydantic/pydantic/blob/fa79d935b45afac09baf52ffde223cabe3254596/pydantic/_internal/_utils.py#L30 )

In the end, I thought that creating a new class was the best solution.

danielgtaylor / python-betterproto