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

Added in a new object id field for mongodb #151

Closed
wants to merge 3 commits into from

Conversation

SkandaPrasad-S
Copy link

@SkandaPrasad-S SkandaPrasad-S commented Mar 3, 2024

Hi everyone -> this is for issue #133

I have been able to add a new pydantic object id field but I am facing some issues with serialisation and testing it

The field is validated correctly when used like

class Something(BaseModel):
    object_id: ObjectIdField

But I am facing some problem in doing Something.model_json_schema(mode="serialization").

Can someone look through my code and help me see what is wrong or what I need to change?

@Ale-Cas
Copy link
Contributor

Ale-Cas commented Mar 4, 2024

Hi everyone -> this is for issue #133

I have been able to add a new pydantic object id field but I am facing some issues with serialisation and testing it

The field is validated correctly when used like

class Something(BaseModel):
    object_id: ObjectIdField

But I am facing some problem in doing Something.model_json_schema(mode="serialization").

Can someone look through my code and help me see what is wrong or what I need to change?

I think to solve this you might need to implement this method:

@classmethod
def __get_pydantic_json_schema__(
        cls, schema: core_schema.CoreSchema, handler: GetJsonSchemaHandler
    ) -> dict[str, Any]: ...

similar to how it is implemented for other types

@Ale-Cas
Copy link
Contributor

Ale-Cas commented Mar 6, 2024

Also can you rename the new files to snake case (mongo_object_id.py and test_mongo_object_id.py) to follow the convention of other file names?

@macintacos
Copy link

Apologies, just wanted to check before I rolled my own implementation - is there still interest in getting this across the line? If @SkandaPrasad-S doesn't have time to get to this I may take a crack at it.

@SkandaPrasad-S
Copy link
Author

Apologies, just wanted to check before I rolled my own implementation - is there still interest in getting this across the line? If @SkandaPrasad-S doesn't have time to get to this I may take a crack at it.

Hey @macintacos - go ahead, I tried to fix this a lot but it did not work. Could you just let me have a dummy commit to your contribution?

Comment on lines +6 to +9
raise RuntimeError(
'The `ObjectIdField` module requires "bson" to be installed. You can install it with "pip install '
'bson".'
)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is correct. It should be pip install pymongo. 😅

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe not mention what should be installed...

@@ -49,7 +49,8 @@ all = [
'pycountry>=23',
'python-ulid>=1,<2; python_version<"3.9"',
'python-ulid>=1,<3; python_version>="3.9"',
'pendulum>=3.0.0,<4.0.0'
'pendulum>=3.0.0,<4.0.0',
'bson>=0.5; python_version>="3.9"',
Copy link
Member

Choose a reason for hiding this comment

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

But here for sure should be pymongo.

@Kludex
Copy link
Member

Kludex commented Aug 23, 2024

I think a lot of ppl would benefit from this, but you can actually open a PR on bson to add the pydantic related function. I have merge rights there.

But... The bson you want is from pymongo - Which I don't have merge rights.

@SkandaPrasad-S
Copy link
Author

I really want to close this off. Can I get some help on the testing side? @sydney-runkle

@SkandaPrasad-S
Copy link
Author

Apologies, just wanted to check before I rolled my own implementation - is there still interest in getting this across the line? If @SkandaPrasad-S doesn't have time to get to this I may take a crack at it.

Also we can brainstorm together to fix it if needed to, please let me know if you want to

@Todor97
Copy link

Todor97 commented Jan 29, 2025

Would love to see this merged!

@Ale-Cas
Copy link
Contributor

Ale-Cas commented Jan 29, 2025

There are failing tests and conflicts in this PR, not sure when this is going to get picked up again but if you want I can open up a PR with my own implementation today based on the comments and suggestions above, should be ready by EOD. I'd also love this see this merged as I'm using heavily both pymongo and pydantic.

@yezz123
Copy link
Collaborator

yezz123 commented Jan 29, 2025

There are failing tests and conflicts in this PR, not sure when this is going to get picked up again but if you want I can open up a PR with my own implementation today based on the comments and suggestions above, should be ready by EOD. I'd also love this see this merged as I'm using heavily both pymongo and pydantic.

Yes sounds good @Ale-Cas if you wanna work on this one 🙏🏻

@Ale-Cas
Copy link
Contributor

Ale-Cas commented Jan 29, 2025

@yezz123 @Kludex Please review: #290

@yezz123
Copy link
Collaborator

yezz123 commented Jan 29, 2025

Closed in favor of this #290

@yezz123 yezz123 closed this Jan 29, 2025
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.

7 participants