Opened 8 years ago

Closed 4 years ago

#26790 closed Cleanup/optimization (needsinfo)

Move BaseUserManager.normalize_email() to AbstractUser

Reported by: Huynh Thanh Tam Owned by: Jacob Walls
Component: contrib.auth Version: dev
Severity: Normal Keywords: normalize_email
Cc: berker.peksag@… Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the current code, normalize_username is placed in AbstractBaseUser, but normalize_email currently stay in BaseUserManager for backward-combatibility.
I suggest we move normalize_email logic to AbstractBaseUser for consistency with normalize_username, and raise warning for the original one.

Change History (11)

comment:1 by Huynh Thanh Tam, 8 years ago

Status: newassigned

comment:2 by Tim Graham, 8 years ago

Summary: Consistency in normalize_email and normalize_usernameMove BaseUserManager.normalize_email() to AbstractUser
Triage Stage: UnreviewedAccepted
Version: 1.10master

If the user has overridden BaseUserManager.normalize_email(), we need to use that implementation during the deprecation period. The idea is similar to the deprecation of ModelAdmin.declared_fieldsets in ebb3e50243448545c7314a1932a9067ddca5960b.

It seems to me the normalize_email() should be moved to AbstractUser (which has the email field) rather than AbstractBaseUser.

comment:3 by Bang Dao, 8 years ago

currently BaseUserManager.normalize_email() is a classmethod, from cls we can't get model attribute like manager_instance.model. Thus when we move normalize_email to AbstractUser, I don't know how to make a extendable call to AbstractUser. normalize_email() from classmethod BaseUserManager.normalize_email() for backward compatibility.

in reply to:  2 comment:4 by Berker Peksag, 8 years ago

Cc: berker.peksag@… added

It seems to me the normalize_email() should be moved to AbstractUser (which has the email field) rather than AbstractBaseUser.

Note that the username field is also defined in AbstractUser but normalize_username() is implemented in AbstractBaseUser.

comment:5 by Tim Graham, 8 years ago

Yes, but AbstractBaseUser.normalize_username() is called on the result of AbstractBaseUser.get_username(). Are you proposing a different organization?

comment:6 by Mariusz Felisiak, 4 years ago

Owner: Huynh Thanh Tam removed
Status: assignednew

comment:7 by Jacob Walls, 4 years ago

Owner: set to Jacob Walls
Status: newassigned

comment:8 by Jacob Walls, 4 years ago

Has patch: set

comment:9 by Jacob Walls, 4 years ago

There are four reasons to move the method to AbstractBaseUser:

  • Adam replied on the mailing list that EMAIL_FIELD is a property on AbstractBaseUser in any event.
  • The reporter was in favor of it for consistency with normalize_username().
  • To pass the suite, as I explained on the mailing list, I needed to support subclasses of AbstractBaseUser that relied on UserManager rather than BaseUserManager to manage objects during the deprecation period. I would have needed to check if normalize_user() was on the manager, and if not, use the implementation on AbstractUser. This seems too involved.
  • This gets us one step closer to deprecating clean() on AbstractUser in favor of AbstractBaseUser.clean(), although I haven't thought through the backward-compatibility ramifications of that ATM.
Version 0, edited 4 years ago by Jacob Walls (next)

comment:10 by Jacob Walls, 4 years ago

A fifth reason, filed under my-patch-would-be-cleaner, is that I can have the old implementation merely call the new implementation. If the new implementation is on AbstractUser, I would encounter a circular import.

comment:11 by Carlton Gibson, 4 years ago

Resolution: needsinfo
Status: assignedclosed
Triage Stage: AcceptedUnreviewed

OK, so Mariusz and I have each come back the PR and the discussion here a number of times over the last few months.

Having conferred we're of the opinion that the proposed change is not worth the disruption: there's clearly confusion is the code as to the levels the attributes and the methods that call them live at but, ultimately the situation isn't much improved after the patch is applied.

There isn't consensus as to which level we should move the methods to (if at all): AbstractBaseUser, AbstractUser, or leave them on BaseUserManager.

(On this last — it's in create() that you want normalize_email() so we go from self.<...> to AbstractBaseUser.<...> in the MyUserManager example in the diff — and I, for one, start wondering if we've really made an improvement... — this is just illustrative — let's not get distracted by it please.)

The various comments comment:2 comment:4 comment:5 comment:9, plus then Adam's reply on the mailing list point to different possible ways we could cut this cake. If I had to choose I'd probably go with Tim's suggestion to move the logic to AbstractUser — but I just don't think it's worth the disruption — and I'm equally drawn to "These methods belong on Manager"…

There's a cluster of code which is more coupled than we'd like. OK. I'm all for clean code, but I'm not sure we have a plan to make it better. (Again, having conferred with Mariusz and having looked at it for some time.) At that point we think the API stability policy dominates. We don't think we should change it without a better analysis. (The current situation is not the end of the world...)

So... I'm going to close this as needsinfo. If someone wants to puzzle this out such that we have an API that's clearly cleaner, plus a nice deprecation path for folks that have implemented on the current situation, and we can get a consensus together on the DevelopersMailingList then, we're very happy to reopen and push forwards. Otherwise, for the moment at least, I think there are probably higher ROI areas we can work on.

I hope that makes sense to all.

Note: See TracTickets for help on using tickets.
Back to Top