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

Use property to alias ForeignKey fields instead of Pydantic alias #1111

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pmdevita
Copy link

Fixes #828 and helps with #469. Full explanation here. Apologies if this explanation isn't quite clear, I'm definitely struggling to explain this properly lol.

Currently for ModelSchema, Ninja uses Pydantic aliases to alias between a ForeignKey field's property name as defined by the user, and the attribute name which holds the ID referenced by the foreign key (author vs author_id). However, this prevents users from using Pydantic's alias_generator with ForeignKey field names.

This PR removes the alias and then changes the property name on the Pydantic model to match the attribute name on the Django model (author_id), which should allow for data dumped out with my_schema.model_dump() to still correctly match up to Django fields as it does now. It then adds property fields to alias between the attribute name and the property name, so any accesses on the model, like my_schema.author, will get aliased to the real property name, my_schema.author_id.

Let me know if there are any changes I need to make. I'm going to also write some tests to polish this off.

@vitalik
Copy link
Owner

vitalik commented Mar 22, 2024

Hi @pmdevita

Yeah I guess it worth trying - just wondering will ti work for case like

class SomeSchema(ModelSchema):
    fkfield_id: int
    
    class Meta:
           model = Some
           fields = ['id', 'fkfield']

@pmdevita
Copy link
Author

pmdevita commented Mar 22, 2024

Yeah I've been thinking over whether this is going to cause some kind of backwards compatibility problem. But I tested the current master branch and it looks like doing this with a schema already causes trouble.

class Author(models.Model):
    id = models.AutoField(primary_key=True)
    name = models.CharField(max_length=50)

    class Meta:
        app_label = "tests"

class Book(models.Model):
    id = models.AutoField(primary_key=True)
    name = models.CharField(max_length=100)
    author = models.ForeignKey(Author, on_delete=models.CASCADE)

    class Meta:
        app_label = "tests"

class BookSchema(ModelSchema):
    author_id: int = Field(...)
    class Meta:
        model = Book
        fields = "__all__"


pprint(BookSchema.json_schema())
test = BookSchema(author_id=1, name="asdfioj")
print(test)

outputs

{'properties': {'author_id': {'title': 'Author', 'type': 'integer'},
                'id': {'anyOf': [{'type': 'integer'}, {'type': 'null'}],
                       'title': 'Id'},
                'name': {'maxLength': 100, 'title': 'Name', 'type': 'string'}},
 'required': ['author_id', 'name', 'author_id'],
 'title': 'BookSchema',
 'type': 'object'}
author_id=1 id=None name='asdfioj' author=1

The schema seems a bit funny, there's only one author_id property but it's mentioned twice in required. We can also see that setting the author_id also sets both.

Same thing here with models.

author_test = Author(name="J. R. R. Tolkien", id=1)
model_test = Book(author=author_test, name="The Hobbit", id=1)
schema_test = BookSchema.from_orm(model_test)
print(schema_test)

outputs

author_id=1 id=1 name='The Hobbit' author=1

It might be reasonable to assume that because this behavior just duplicates data, it probably isn't useful and it's unlikely anyone would be relying on it.

Here is the output from that test on my branch for comparison.

{'properties': {'author_id': {'title': 'Author Id', 'type': 'integer'},
                'id': {'anyOf': [{'type': 'integer'}, {'type': 'null'}],
                       'title': 'Id'},
                'name': {'maxLength': 100, 'title': 'Name', 'type': 'string'}},
 'required': ['author_id', 'name'],
 'title': 'BookSchema',
 'type': 'object'}
author_id=1 id=None name='asdfioj'
author_id=1 id=1 name='The Hobbit'

@pmdevita pmdevita marked this pull request as ready for review March 22, 2024 21:55
@pmdevita
Copy link
Author

I left the class Config in the docs but I just realized Pydantic deprecated that, is model_config now the recommended way to go for Ninja?

@pmdevita
Copy link
Author

pmdevita commented Apr 2, 2024

Anything else I can do to help move this along?

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.

[BUG] ForeignKey field in modelSchema do not use the related alias_generator
2 participants