furious-luke / django-address

A Django address model and field. Addresses may be specified by address components or by performing an automatic Google Maps lookup.
BSD 3-Clause "New" or "Revised" License
431 stars 181 forks source link

Address save via admin saves the primary key into raw column, and additionally creates a bare new object #177

Open jheld opened 2 years ago

jheld commented 2 years ago

Looks like the general culprit is from https://github.com/furious-luke/django-address/blame/develop/address/models.py#L138 which is based on the "ID" coming in as a str, as opposed to an int.

I have a fix temporarily in-place via subclassing in my project.

It looks similar to:

    # If we have an integer, assume it is a model primary key.
    elif isinstance(value, int) or (isinstance(value, str) and value.isnumeric()):
        obj = Address.objects.filter(pk=value).first() or value
        return obj    # If we have an integer, assume it is a model primary key.

    # A string is considered a raw value.
    elif isinstance(value, str):
        obj = Address(raw=value)
        obj.save()
        return obj
furious-luke commented 2 years ago

Hi @jheld, thanks for the report! Would you be able to give me the steps required to reproduce the issue you're experiencing?

jheld commented 2 years ago

Ok, I think I have isolated the scenario further.

In my case, I am using raw_id_fields and it includes the "address" foreign key field on my model.

In the admin, if I simply save and continue editing, but don't actually change anything (the address must already have a foreign key value in this situation), then the issue will arise. Every time we submit, even with no change at all, there is a new address instance created, and the foreign key's value will simply increase, while it's "human readable" (e.g. raw field by default as __str__) format will show what happens to be in the raw which is only the PK, due to Address(raw=value).

My current iteration of the fix is effectively this (note I am wrapping the behavior of the function more explicitly):

def address_to_python(value):
    if isinstance(value, int) or (isinstance(value, str) and value.isnumeric()):
        obj = Address.objects.filter(pk=value).first() or value
        return obj
    else:
        return address.models.to_python(value)

When we use the raw_id_fields, django will send down the address field in the form with a string-value of the ID. When it's not a raw id field, we get the full break down of fields and it's thus converted to a dict coming into the to_python function and treated more correctly.

I did fork the project and write up a "successful" failure case (e.g. person.address.id == address.id + 1) when assigning person.address = str(address.id) and saving, to showcase that this is actually possible outside of the admin as well, by usage of:

class AddressDescriptor(ForwardManyToOneDescriptor):
    def __set__(self, inst, value):
        super(AddressDescriptor, self).__set__(inst, to_python(value))

But given that I was able to isolate the issue more explicitly, I don't think my grass-roots test is all the valuable anymore (e.g. we'd want a test with the fix in place, not the problem itself being tested).

furious-luke commented 2 years ago

Thanks for the response! I think I understand the situation; using raw_id_fields will indeed return a string that isn't converted automatically to a number, as would be the case with the select dropdown. Leave it with me, I'll add some tests to the project and then submit a fix.