refactor(socialaccount): Avoid database lookup at is_existing

[jonasN5]

Simple PR that makes sure a newly created user model does not have a pk assigned, which is otherwise automatically assigned if the field is defined on the model and has a default value set. For example, this model would currently have a pk assigned:

class User(AbstractBaseUser):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)

Thus, this code does not behave as expected:

    @property
    def is_existing(self):
        """When `False`, this social login represents a temporary account, not
        yet backed by a database record.
        """
        if self.user.pk is None:
            return False

[coveralls]

coverage: 95.602%. remained the same
when pulling d1142e9 on jonasN5:main
into b1786ed on pennersr:main.

[pennersr]

You did not copy the full code (either that, or, you are using an outdated version?). The full code is:

        if self.user.pk is None:
            return False
        return get_user_model().objects.filter(pk=self.user.pk).exists()

So, even if pk is set, the .exists() check will give the proper answer. Therefore, the issue you are solving is already handled. Please upgrade.

[jonasN5]

I do have it. However, the current code will then perform multiple database calls (I logged 2) where we are actually 100% certain that the user does not exist at this point because the object was just created. The change I'm proposing handles the id+default scenario. It seems to me the intent was to create a User object without a pk set, otherwise why would the line if self.user.pk is None: be there?

[pennersr]

The intention of this PR was not fully clear to me:

Thus, this code does not behave as expected:

Suggests that the code is wrong, where it is actually correct.

If we are talking about # of DB queries, that is a different matter. Though, perhaps it is better then to check user._state.adding instead of dealing with None pks indirectly.

[jonasN5]

The intention of this PR was not fully clear to me:

Thus, this code does not behave as expected:

Suggests that the code is wrong, where it is actually correct.

If we are talking about # of DB queries, that is a different matter. Though, perhaps it is better then to check user._state.adding instead of dealing with None pks indirectly.

Thanks for the pointer. I didn't look into what this does so I'll check it out and update the PR.

[pennersr]

I suspect -- haven't tested it myself -- that only doing return not self.user._state.adding would suffice.

[pennersr]

I really don't think it is necessary to keep track of this state ourselves, as Django already does that for you:

instance._state.adding

[pennersr]

Hmm, while this works in most cases, I am afraid it can subtly break. For example:

>>> user=User.objects.first()
>>> user._state.adding
False
>>> user2=deserialize_instance(User, serialize_instance(user))
>>> user2._state.adding
True

Now, while that serialization can be fixed on this end, if projects are doing customized things things go wrong.

All in all, I don't think the costs outweigh the benefits of this one query.

[jonasN5]

In that case, my initial suggestion is fine? e.g. pk=None

[pennersr]

If you have a Django UUIDField named foo with a default=uuid.uuid4, and you override force foo to be None, and then later on save it, doesn't that result in an error? At what time is the default applied once more?

In any case, seems a bit tricky to me. Safer would be:

    @cached_property
    def is_existing(self):

[jonasN5]

I tried cached_property, a bunch of tests failed because of it. But no, I've tested that behavior. Setting it to None is fine, the value is generated when you save. I'll find the line.

[jonasN5]

******* Loading settings for environment: local *******
Python 3.12.3 (main, Apr 27 2024, 19:00:21) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
(InteractiveConsole)
>>> from user.models import User
>>> u = User(pk=None, email="someemail")
>>> u.pk
>>> u.save()
>>> u.pk
UUID('13018c30-520b-456e-8875-f239eab86fa0')
>>> 

[pennersr]

Interesting, the behavior is different for non PKs. Take this model:

class MyUUIDModel(models.Model):
    id = models.UUIDField(primary_key=True, default=uuid.uuid4, editable=False)
    foo = models.UUIDField(default=uuid.uuid4, blank=False, null=False)

>>> MyUUIDModel(foo=None).save()
django.db.utils.IntegrityError: NOT NULL constraint failed: demo_myuuidmodel.foo

[pennersr]

I tried cached_property, a bunch of tests failed because of it.

That may be because of tests doing non-standard things. Will have a look at a later time.

I am still a bit worried about pk=None. While it indeed works fine with models.UUIDFIeld there is no way to tell for sure that this just works with all the non-standard primary key fields people are using (e.g. hashidfield and so on).

[jonasN5]

https://github.com/django/django/blob/main/django/db/models/fields/__init__.py#L742
This is where it happens.
It's called here: https://github.com/django/django/blob/main/django/db/models/base.py#L1081
So yes, this is always done for pks, regardless of the type.

[pennersr]

Adjusting new_user() is problematic as that won't work when projects override that method. I've added this now, that is in any case safe:

2021e0b

[jonasN5]

In my opinion, when you override the default it simply becomes your responsibility to keep any of the original intent (if you want it). Adding a bunch of boilerplate code to achieve the same result reached by the default doesn't make sense to me.
Anyhow, that's just my opinion. Thanks for the implementation.