zsoldosp / django-currentuser

Conveniently store reference to request user on thread/db level.
BSD 3-Clause "New" or "Revised" License
141 stars 40 forks source link

Warning running `makemigrations` after updating to Django 3.1 and django-currentuser 0.5.1 #43

Open fgs-dbudwin opened 3 years ago

fgs-dbudwin commented 3 years ago

Environment:

Problem:

When I run python manage.py makemigrations I get the following warning. This warning is new after updating to the latest version of Django and django-currentuser. If I revert Django to 3.0.X the warning will go away.

/home/admin/.local/share/virtualenvs/Project-HFzz1tdt/lib/python3.8/site-packages/django_currentuser/db/models/fields.py:57: UserWarning: You passed an argument to CurrentUserField that will be ignored. Avoid args and following kwargs: default, null, to.
  warnings.warn(self.warning)

I have one reference to CurrentUserField in my project:

from django.db import models
from django_currentuser.db.models import CurrentUserField

class UserTrackingModel(models.Model):
    created_by = CurrentUserField()

    class Meta:
        abstract = True

I use UserTrackingModel as a base class for some of my models, like so:

from camp.db_models import UserTrackingModel

class Foo(TimeStampedModel, UserTrackingModel):
    name = models.CharField(default="", max_length=200, unique=True)
    thing = models.ForeignKey(Thing, on_delete=models.PROTECT, null=False)

Desired Outcome

I would like to be able to generate migrations without getting a warning that could potentially turn into a bigger problem down the road.

belugame commented 3 years ago

Thank you for the detailed bug report. I assume Django 3.1 passes a new built-in kwarg which CurrentUser should expect but does not. Though we will need to look into the docs/source for that since we don't use 3.1 yet.

N1K1TAS95 commented 3 years ago

I have the same warning (I guess):

/usr/local/lib/python3.9/site-packages/django_currentuser/db/models/fields.py:57: UserWarning: You passed an argument to CurrentUserField that will be ignored. Avoid args and following kwargs: default, null, to.
warnings.warn(self.warning)

I'm using this setup:

avinashjoshi commented 3 years ago

Same issue here! It seems like this issues is specific to when using a custom user model. Did some digging into this by adding print statements here and found that the to parameter passed in kwargs different from that in self.defaults:

kwargs: {..., "to": "accounts.user"}
self.defaults: {..., "to": "accounts.User"}

Updating AUTH_USER_MODEL in settings.py to the following fixed the issue (note the lowercase u in user):

AUTH_USER_MODEL = "accounts.user"

Now the bigger question is which of the following is correct:

Any thoughts on what is the right approach to fix this issue?

N1K1TAS95 commented 3 years ago

Same issue here! It seems like this issues is specific to when using a custom user model. Did some digging into this by adding print statements here and found that the to parameter passed in kwargs different from that in self.defaults:

kwargs: {..., "to": "accounts.user"}
self.defaults: {..., "to": "accounts.User"}

Updating AUTH_USER_MODEL in settings.py to the following fixed the issue (note the lowercase u in user):

AUTH_USER_MODEL = "accounts.user"

Now the bigger question is which of the following is correct:

  • AUTH_USER_MODEL = "accounts.User" (this is what is recommended in django docs) OR
  • AUTH_USER_MODEL = "accounts.user"

Any thoughts on what is the right approach to fix this issue?

I cannot say anything about this solution because I'm using a custom model for user AUTH_USER_MODEL = "core.User"

fgs-dbudwin commented 3 years ago

@avinashjoshi and @N1K1TAS95 are you using version 0.5.2 of this library? I submitted a fix and had it merged, I believe this issue should be closed unless it still appears to happen on the latest version.

See: https://github.com/PaesslerAG/django-currentuser/releases/tag/0.5.2

N1K1TAS95 commented 3 years ago

@avinashjoshi and @N1K1TAS95 are you using version 0.5.2 of this library? I submitted a fix and had it merged, I believe this issue should be closed unless it still appears to happen on the latest version.

See: https://github.com/PaesslerAG/django-currentuser/releases/tag/0.5.2

I tried to reinstall the package (last version 0.5.2), but I still have the same warning.

Maybe it's my fault?

Currently I'm using the CurrentUserFiled as this:

user = CurrentUserField(
        verbose_name='Creato da',
        on_delete=models.PROTECT
)

Maybe I'm missing some args or kwargs?

cccaballero commented 3 years ago

I can confirm that this is happening using 0.5.2 with Django 3.1.5. I am also using django-allauth (I'm not sure if it has something to do)

EthanZeigler commented 3 years ago

@fgs-dbudwin any updates on this?

aidanlister commented 3 years ago

Changing AUTH_USER_MODEL = "accounts.User" to lowercase fixed my warnings too.

EthanZeigler commented 2 years ago

Changing AUTH_USER_MODEL = "accounts.User" to lowercase fixed my warnings too.

I mean, it's a solution, but I don't like this given that django recommends the accounts.User pattern.

clovisp commented 2 years ago

Same problem here, any idea when it could be solved?

jongracecox commented 2 years ago

I am also hitting this issue, but I am not using any custom user model. I'm using django==3.1.5 and django-currentuser==0.5.3. I do have a custom authentication middleware, but I don't think that would factor in.

jongracecox commented 2 years ago

As a test I updated the _warn_for_shadowing_args() function in django_currentuser.db.models.fields.py to include extra information in the warning message:

    def _warn_for_shadowing_args(self, *args, **kwargs):
        if args:
            warnings.warn(self.warning)
        else:
            for key in set(kwargs).intersection(set(self.defaults.keys())):
                if not kwargs[key] == self.defaults[key]:
                    warnings.warn(
                        self.warning + f" Unexpected arg was: {key}={repr(kwargs[key])}."
                                       f" Default is: {repr(self.defaults[key])}"
                    )
                    break

This results in:

Operations to perform:
  Apply all migrations: admin, auth, client_dis, contenttypes, drs, portal_home, sessions
.../venv/lib/python3.9/site-packages/django_currentuser/db/models/fields.py:65: UserWarning: You passed an argument to CurrentUserField that will be ignored. Avoid args and following kwargs: default, null, to. Unexpected arg was: to='auth.user'. Default is: 'auth.User'

or

.../venv/lib/python3.9/site-packages/django_currentuser/db/models/fields.py:65: UserWarning: You passed an argument to CurrentUserField that will be ignored. Avoid args and following kwargs: default, null, to. Unexpected arg was: null=False. Default is: True

I wonder whether it would be nice to include this as an update so it's more obvious which unexpected args were passed.

Running a Django shell I can see that my user model is set to auth.User, and updating the settings to use auth.user does make the warning go away, but this doesn't seem like a good fix.

Here's an option for handling the simple difference in case, in the CurrentUserField class in django_currentuser.db.models.fields.py:

    def __init__(self, *args, **kwargs):
        self.on_update = kwargs.pop("on_update", False)

        # If `to` is present in kwargs, and the same when ignoring case then
        # update `to` to use the defaults.
        # Fix for https://github.com/PaesslerAG/django-currentuser/issues/43
        if "to" in kwargs and kwargs["to"].lower() == self.defaults['to'].lower():
            kwargs["to"] = self.defaults['to']

        self._warn_for_shadowing_args(*args, **kwargs)
        ...

It does feel a little hacky, but it should be safe, and prevent the warning for users that are using the default user model.