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

PLACEHOLDER test sometimes fails when using pydantic #606

Closed
3 tasks done
AdrienVannson opened this issue Sep 4, 2024 · 2 comments · Fixed by #611
Closed
3 tasks done

PLACEHOLDER test sometimes fails when using pydantic #606

AdrienVannson opened this issue Sep 4, 2024 · 2 comments · Fixed by #611
Labels
bug Something isn't working investigation needed

Comments

@AdrienVannson
Copy link
Contributor

AdrienVannson commented Sep 4, 2024

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: pydantic/pydantic#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: [email protected]
License: MIT
Location: XXX
Requires: grpclib, python-dateutil, typing-extensions
Required-by: pygrpc-poc

Checklist

  • I have searched the issues for duplicates.
  • I have shown the entire traceback, if possible.
  • I have verified this issue occurs on the latest prelease of betterproto which can be installed using pip install -U --pre betterproto, if possible.
@AdrienVannson AdrienVannson added bug Something isn't working investigation needed labels Sep 4, 2024
@Gobot1234
Copy link
Collaborator

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
Copy link
Contributor Author

AdrienVannson commented Sep 9, 2024

I submitted a PR: #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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working investigation needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants