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.

Sign up for GitHub

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

Jump to bottom

Fix PydanticObjectId fields being parsed into str #1060

Merged
merged 1 commit into from
Oct 25, 2024
Merged

Fix PydanticObjectId fields being parsed into str #1060

merged 1 commit into from
Oct 25, 2024

Conversation

07pepa
Copy link
Member

@07pepa 07pepa commented Oct 21, 2024 •
edited by adeelsohailahmed
Loading

full implementation of #1054 that passes test and include new ones

Fixes #940

@07pepa 07pepa mentioned this pull request Oct 21, 2024
Closed
Riverfount
Riverfount previously approved these changes Oct 21, 2024
View reviewed changes
Copy link

@Riverfount Riverfount left a comment

Choose a reason for hiding this comment

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

Great Job.

CAPITAINMARVEL
CAPITAINMARVEL previously approved these changes Oct 21, 2024
View reviewed changes
adeelsohailahmed
adeelsohailahmed previously requested changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@adeelsohailahmed adeelsohailahmed left a comment

Choose a reason for hiding this comment

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

Add a descriptive title to your PR, please. This title is used in the changelog. Adding a vague title doesn't tell us what was changed.

beanie/odm/fields.py Outdated Show resolved Hide resolved
@07pepa
Copy link
Member Author

07pepa commented Oct 21, 2024

Add a descriptive title to your PR, please. This title is used in the changelog. Adding a vague title doesn't tell us what was changed.

I did not know

@07pepa 07pepa dismissed stale reviews from CAPITAINMARVEL and Riverfount via 34dad23 October 21, 2024 19:20
@07pepa 07pepa changed the title fix #940 Fix JSON Parsing of PydanticObjectId Oct 21, 2024
@07pepa 07pepa changed the title Fix JSON Parsing of PydanticObjectId Fix PydanticObjectId fields being parsed into str Oct 21, 2024
adeelsohailahmed
adeelsohailahmed approved these changes Oct 21, 2024
View reviewed changes
Copy link
Contributor

@adeelsohailahmed adeelsohailahmed left a comment

Choose a reason for hiding this comment

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

Thanks for the changes. LGTM.

staticxterm
staticxterm approved these changes Oct 23, 2024
View reviewed changes
Copy link

@staticxterm staticxterm left a comment

Choose a reason for hiding this comment

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

Just a bit worried if this would break other serialization/deserialization use cases for when Document.id is not an instance of a PydanticObjectId or string.

Otherwise looks good. Tested this a bit and it worked for me...

@mg3146
Copy link

mg3146 commented Oct 23, 2024

Just a bit worried if this would break other serialization/deserialization use cases for when Document.id is not an instance of a PydanticObjectId or string.

Otherwise looks good. Tested this a bit and it worked for me...

Just so I understand, would that be the case where people explicitly set the field "id" for some other use outside of mongo object id?

Riverfount
Riverfount approved these changes Oct 24, 2024
View reviewed changes
Copy link

@Riverfount Riverfount left a comment

Choose a reason for hiding this comment

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

LGTM

@07pepa 07pepa merged commit 66d78bf into BeanieODM:main Oct 25, 2024
51 checks passed
@07pepa 07pepa deleted the fix-#940 branch October 25, 2024 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticxterm staticxterm staticxterm approved these changes

@Riverfount Riverfount Riverfount approved these changes

@adeelsohailahmed adeelsohailahmed adeelsohailahmed approved these changes

@ayusshsoarav ayusshsoarav ayusshsoarav approved these changes

@CAPITAINMARVEL CAPITAINMARVEL CAPITAINMARVEL approved these changes

@LawMixer LawMixer Awaiting requested review from LawMixer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] PydanticObjectId isn't properly configured when deserializing from JSON
7 participants
@07pepa @mg3146 @staticxterm @Riverfount @adeelsohailahmed @ayusshsoarav @CAPITAINMARVEL