Use property to alias ForeignKey fields instead of Pydantic alias

[pmdevita]

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]

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]

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]

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]

Anything else I can do to help move this along?

[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!

[Tatsh]

@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": ""}
}

[pmdevita]

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

[SaschaKrug]

Is there any reason this hasn't been merged? The handling of alias generators is very buggy currently with foreign keys and it looks like this PR would fix it?

[pmdevita]

@vitalik Is there anything you'd like to me to change with this? It would be nice to have it merged

[pmdevita]

I have made a mistake by leaking unrelated commits from a fork with my rebase earlier, I think it also accidentally backlinked some stuff too in the repo. I'm really sorry about that, I don't think there is but I'll do anything I can to fix it.

EDIT: It looks like it mostly affected 1, 2, and 4. I'll be more careful from now on.

[pmdevita]

Some extra context I wrote up since it's been a while and I don't remember this all to well anymore.

Sort of an example just with Pydantic, the old way would look like this

class MySchema(ModelSchema):
    author: int = Field(..., alias="author_id")  # generated by ModelSchema
    
    class Meta:
        model = Book
        fields = ["author"]

which would of course then cause a problem with use of automatic alias generation. My implementation did this

class MySchema(ModelSchema):
    author_id: int = Field(...)  # generated by ModelSchema
    
    @property                    # generated by ModelSchema
    def author(self):
        return author

    @author.setter               # generated by ModelSchema
    def author(self, value):
        self.author = value
    
    class Meta:
        model = Book
        fields = ["author"]

which would then free up alias to be changed.

In the time since I've written this, I did run into a similar issue to what vitalik states here #1111 (comment). The trouble is, *_id has to be the field's real name in order for that to be used in alias generation. But additionally as I stated in my follow up comment, and as I found out, you were already going to be in trouble if you were trying to do that, even without this patch being merged.

There is a very similar problem that I realized with this patch right now though, which is what happens if you do this.

class MySchema(ModelSchema):
    author: AuthorSchema   # my manually added field

    author_id: int = Field(...)  # generated by ModelSchema
    
    # @property                    # generated by ModelSchema?
    # def author(self):
    #     return author

    # @author.setter               # generated by ModelSchema?
    # def author(self, value):
    #     self.author = value
    
    class Meta:
        model = Book
        fields = ["author"]

We can fix this by making sure the property are not added so they don't override Author. I'll update this PR to fix that and add a test.

[yetty]

Hi @vitalik, any updates on this pull request? It seems like there are several people experiencing issues related to alias generators and foreign keys. Just curious if there's anything blocking this from being merged. Thanks!