Django-guardian: ANONYMOUS_USER_NAME None value

[g-as]

Am I missing something or with this line in guardian.conf.settings

if ANONYMOUS_USER_NAME is None:
    ANONYMOUS_USER_NAME = 'AnonymousUser'

ANONYMOUS_USER_NAME cannot ever be equal to None, thus voiding the fact that it is supposed to optional. Looks to me that this was not desired.

Cheers

[brianmay]

Yes, you are right, that does look dodgy. Need a way to be able to default to 'AnonymousUser' without preventing it being explicitly set to None.

[brianmay]

What if I changed the code to the following? Any better? This should set the default value if nothing specified and allow explicitly setting it to None.

try:
    ANONYMOUS_USER_NAME = settings.ANONYMOUS_USER_NAME
except AttributeError:
    try:
        ANONYMOUS_USER_NAME = settings.ANONYMOUS_DEFAULT_USERNAME_VALUE
        warnings.warn("The ANONYMOUS_DEFAULT_USERNAME_VALUE setting has been renamed to ANONYMOUS_USER_NAME.", DeprecationWarning)
    except AttributeError:
        ANONYMOUS_USER_NAME = "AnonymousUser"

[g-as]

I'd say just remove the two line I commented.

Since ANONYMOUS_USER_NAME is actually the value for the USERNAME_FIELD, ANONYMOUS_USER_NAMEcould be an email or whatever else. Defaulting to "AnonymousUser"is thus misleading. Furthermore, ANONYMOUS_USER_ID was triggering the feature and ANONYMOUS_DEFAULT_USERNAME_VALUE served as username. But now, they have been merger into one setting, which serves as trigger and value. So there is no point in providing a default value.

So I would stick to:

ANONYMOUS_USER_NAME = getattr(settings, 'ANONYMOUS_USER_NAME', None)

if ANONYMOUS_USER_NAME is None:
    ANONYMOUS_USER_NAME = getattr(settings, 'ANONYMOUS_DEFAULT_USERNAME_VALUE', None)
    if ANONYMOUS_USER_NAME is not None:
        warnings.warn("[...]", DeprecationWarning)

and remove the aformentioned two lines.

I have mixed feelings about the try...exceptsyntax because, even though the truth is not a matter of number, I have never seen it used for settings. The classic <SETTING_NAME> = getattr(settings, "<SETTING_NAME>", <default_value>) usually works pretty well.

[brianmay]

If we don't provide a default value for ANONYMOUS_USER_NAME, this is going to break existing applications that assume a default value is going to be provided. i.e. we have changed the behaviour for when the ANONYMOUS_USER_NAME setting is not given.

[g-as]

Then why not bumping to 1.5.0, removing ANONYMOUS_DEFAULT_USERNAME_VALUE and the default value, and warning about breaking changes?

[brianmay]

I believe the intention was to allow running Django Guardian without it automatically creating an anonymous user. Do you have a use case for this? Why not just ignore the anonymous user if you don't want it?

I think creating an anonymous user by default is a good thing, I don't see any reason why we should change this. However we do need a default value of ANONYMOUS_USER_NAME for this to work. If the default is inappropriate for your application, you can always change it.

The try ... except syntax is good because it allows as to distinguish the cases of "Was this setting not given?" and "Was this setting given a null value?" - You might be able to use getattr for this, however I have found this can complicated rather quickly, particularly if you want to keep the DeprecationWarning. I am not sure what you mean by "even though the truth is not a matter of number".

[g-as]

What I meant was it's not because everyone does one thing that it's the right/better way.

Forcing the creation of an Anonymous User could be a way to go, but then the docs should be updated.

Regarding my situation, I'm using an email field as the default USERNAME_FIELD, hence my initial reluctance to create the anonymous user instance. But I can manage.

Cheers. And thanks for letting me bust your b*lls.

[edufelipe]

Guys, I don't really think that requiring the creation of an anonymous user is a good idea. I've been using django-guardian for a long time and having a anonymous user was never required, so why change that now?

[brianmay]

So my solution involving try ... except was given 2 thumbs up, however where I justify this solution I was given 2 thumbs down. Confusing. As I really don't see any downside, and I believe this is the simplest way of resolving the regression without adding new regressions, I am going to apply this solution.

[edufelipe]

@brianmay Sorry about the confusion. :+1: for the fix, though!