Change the way values FK fields on a custom User model are handled for the 'createsuperuser' command
Hello Django developpers, Following the discussion I had with Mariusz Felisiak on PR #14913 <https://github.com/django/django/pull/14913/files/f7492ae19586e6fd76e235cbbe902b5da657423e#diff-e5abf5944f650c7b4cebaee079168b66a303f57e96c2cf441a0a5a0251e68524>, I would like to suggest a small change in the way the createsuperuser command processes values for foreign key fields of custom User models. This all started as I was trying to override the createsuperuser command for a Django project I work on. This project has a custom user model with a FK field named `organisation`. As I was working on that, I noticed that, if I passed an instance of the Organisation model for the field organisation, I got a TypeError because createsuperuser performs a field.clean on the value which tries to convert a model into an integer. But if I pass an integer, I get a ValueError, because later in the code, the command calls the the AbstractUser constructor which expects an instance of Organisation for the organisation field. It felt like a bug, so I opened #33151 <https://code.djangoproject.com/ticket/33151> and proposed to fix this in a PR that simply wraps the input value in a model <https://github.com/django/django/pull/14913/files#diff-008fad28cbf57009885eb4ba44f8460c9d58088af8bd9be3db2046cac37744f1R142>. But, during the review, Mariusz Felisiak informs me that this behaviour is documented <https://docs.djangoproject.com/en/3.2/topics/auth/customizing/#django.contrib.auth.models.CustomUserManager> as it is advised to always write a custom manager with the create_user and create_superuser methods and any change here is a breaking change that should be discussed on the developer's mailing list. Which is why I would like to make my case here. I would like to stress how easy it is to miss that part of the documentation and run into this unexpected behaviour. Everywhere in the framework, when I use the name of an FK field, an instance of the model is expected. This is true for model constructors and every method of the ORM. Which is why it was disturbing to me seeing createsuperuser break this convention and use the name of the FK field to pass the primary key. This means, every time I will use a custom user model in combination to REQUIRED_FIELDS, I will have to write a custom manager with create_user and create_superuser methods that do this: class CustomUserManager(UserManager): def __normalize_fields(self, extra_fields: dict): for field_name in extra_fields.keys(): field = self.model._meta.get_field(field_name) field_value = extra_fields[field_name] if field.many_to_one and not isinstance( field_value, field.remote_field.model ): field_value = ( int(field_value) if not isinstance(field_value, int) else field_value ) extra_fields[field_name] = field.remote_field.model(field_value) def create_user(self, username, email=None, password=None, **extra_fields): self.__normalize_fields(extra_fields) return super().create_user(username, email, password, **extra_fields) def create_superuser(self, username, email=None, password=None, **extra_fields): self.__normalize_fields(extra_fields) return super().create_superuser(username, email, password, **extra_fields) Even though it's this only thing it does. IMHO, the createsuperuser command could be — and should be — doing this itself. BTW, it's what's being done in interactive mode <https://github.com/django/django/blob/main/django/contrib/auth/management/commands/createsuperuser.py#L142> to check the validity of the user input, even though the wrapping is discarded immediatly after and only the raw value is transmitted. I don't really see why the command performs a check in this situation but not always. -- /Cordialement/Regards, Christophe Henry/ — Christophe Henry https://christophe-henry.dev/ Dev. Python/JS/Android & devops Timon-Ethics / Coopaname <http://www.coopaname.coop/> -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/ecde841c-0a68-681a-be85-3e4242385367%40timon-ethics.fr.
Re: Change the way values FK fields on a custom User model are handled for the 'createsuperuser' command
Hi, This discussion does not seems to attract much attention but I still feel this is a nice improvement to have. /Cordialement/Regards, Christophe Henry/ — Christophe Henry https://christophe-henry.dev/ Dev. Python/JS/Android & devops Timon-Ethics / Coopaname <http://www.coopaname.coop/> Le 08/10/2021 à 13:55, Christophe Henry a écrit : Hello Django developpers, Following the discussion I had with Mariusz Felisiak on PR #14913 <https://github.com/django/django/pull/14913/files/f7492ae19586e6fd76e235cbbe902b5da657423e#diff-e5abf5944f650c7b4cebaee079168b66a303f57e96c2cf441a0a5a0251e68524>, I would like to suggest a small change in the way the createsuperuser command processes values for foreign key fields of custom User models. This all started as I was trying to override the createsuperuser command for a Django project I work on. This project has a custom user model with a FK field named `organisation`. As I was working on that, I noticed that, if I passed an instance of the Organisation model for the field organisation, I got a TypeError because createsuperuser performs a field.clean on the value which tries to convert a model into an integer. But if I pass an integer, I get a ValueError, because later in the code, the command calls the the AbstractUser constructor which expects an instance of Organisation for the organisation field. It felt like a bug, so I opened #33151 <https://code.djangoproject.com/ticket/33151> and proposed to fix this in a PR that simply wraps the input value in a model <https://github.com/django/django/pull/14913/files#diff-008fad28cbf57009885eb4ba44f8460c9d58088af8bd9be3db2046cac37744f1R142>. But, during the review, Mariusz Felisiak informs me that this behaviour is documented <https://docs.djangoproject.com/en/3.2/topics/auth/customizing/#django.contrib.auth.models.CustomUserManager> as it is advised to always write a custom manager with the create_user and create_superuser methods and any change here is a breaking change that should be discussed on the developer's mailing list. Which is why I would like to make my case here. I would like to stress how easy it is to miss that part of the documentation and run into this unexpected behaviour. Everywhere in the framework, when I use the name of an FK field, an instance of the model is expected. This is true for model constructors and every method of the ORM. Which is why it was disturbing to me seeing createsuperuser break this convention and use the name of the FK field to pass the primary key. This means, every time I will use a custom user model in combination to REQUIRED_FIELDS, I will have to write a custom manager with create_user and create_superuser methods that do this: class CustomUserManager(UserManager): def __normalize_fields(self, extra_fields: dict): for field_name in extra_fields.keys(): field = self.model._meta.get_field(field_name) field_value = extra_fields[field_name] if field.many_to_one and not isinstance( field_value, field.remote_field.model ): field_value = ( int(field_value) if not isinstance(field_value, int) else field_value ) extra_fields[field_name] = field.remote_field.model(field_value) def create_user(self, username, email=None, password=None, **extra_fields): self.__normalize_fields(extra_fields) return super().create_user(username, email, password, **extra_fields) def create_superuser(self, username, email=None, password=None, **extra_fields): self.__normalize_fields(extra_fields) return super().create_superuser(username, email, password, **extra_fields) Even though it's this only thing it does. IMHO, the createsuperuser command could be — and should be — doing this itself. BTW, it's what's being done in interactive mode <https://github.com/django/django/blob/main/django/contrib/auth/management/commands/createsuperuser.py#L142> to check the validity of the user input, even though the wrapping is discarded immediatly after and only the raw value is transmitted. I don't really see why the command performs a check in this situation but not always. -- /Cordialement/Regards, Christophe Henry/ — Christophe Henry https://christophe-henry.dev/ Dev. Python/JS/Android & devops Timon-Ethics / Coopaname <http://www.coopaname.coop/> -- You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group. To unsubscribe from this
