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

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

Open
jheld opened this issue Jun 27, 2022 · 3 comments
Assignees

Comments

@jheld
Copy link

jheld commented Jun 27, 2022

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 furious-luke self-assigned this Jun 28, 2022
@furious-luke
Copy link
Owner

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

@jheld
Copy link
Author

jheld commented Jun 28, 2022

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
Copy link
Owner

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.

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

No branches or pull requests

2 participants