django-ninja icon indicating copy to clipboard operation
django-ninja copied to clipboard

Use property to alias ForeignKey fields instead of Pydantic alias

Open pmdevita opened this issue 1 year ago • 12 comments

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.

pmdevita avatar Mar 22 '24 03:03 pmdevita

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']

vitalik avatar Mar 22 '24 07:03 vitalik

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 avatar Mar 22 '24 18:03 pmdevita

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 avatar Mar 23 '24 17:03 pmdevita

Anything else I can do to help move this along?

pmdevita avatar Apr 02 '24 14:04 pmdevita

@vitalik Could you take a look at this again and let me know if I need to fix anything? I'd really like to get this merged since I'm using to_camel for all of my API schemas. Thank you!

pmdevita avatar Jul 11 '24 14:07 pmdevita

@pmdevita does this fix this issue?

class SomeSchema(Schema):
    field: datetime = Field(None, alias='user.date')
    
class AnotherSchema(Schema):
    some: SomeSchema = None

Outputs:

{
  "some": {"user.date": ""}
}

But we want:

{
  "some": {"field": ""}
}

Tatsh avatar Aug 05 '24 19:08 Tatsh

It doesn't, this solves an issue specific to foreign keys and alias generators. I'm not sure why you're aliasing like that but maybe AliasPath and AliasChoice can help you if you're trying to access a property on the data being validated

pmdevita avatar Aug 05 '24 20:08 pmdevita