silviogutierrez / reactivated

https://www.reactivated.io
MIT License
671 stars 20 forks source link

Best strategy for implementing contrib.auth context processor by default? #183

Open minism opened 2 years ago

minism commented 2 years ago

Hi there,

Firstly thanks for your great work on this package, this is giving us the best of both worlds for prototyping, I love how productive it feels!

I couldn't find in the documentation a recommendation for this, so I'm curious what your thoughts are. I'd like to get the contrib.auth context processor working by default in reactivated.

I was able to do this by overriding reactivated/serialization/context_processors.py and injecting:

from django.contrib.auth.models import User
from reactivated import Pick

...

class AuthProcessor(NamedTuple):
    user: Pick[User, "username", ...]

TYPE_HINTS = {
    ...
    "django.contrib.auth.context_processors.auth": {
      "return": AuthProcessor
    },
}

Which seems to work, however I'm not sure if its appropriate to import contrib.auth.models here.

I suppose the question is, is there a better way to approach this, or would the recommendation be simply to forego the contrib.auth context processor entirely and re-implement it yourself (with proper typing)?

silviogutierrez commented 2 years ago

Great question. It's not built in because how people serialize User varies widely.

There are other parts in the library that let you "register" a type for something that is already defined. See reactivated/serialization/widgets.py for example. But no such thing exists for context processors yet.

I think the best approach is just to let you register django.contrib.auth.context_processors.auth with a return type, without having to hack the package. Then you can do whatever you want.

Or do you feel strongly like there should be a default built in, as there is for other ones like staticfiles? If so, what fields would be picked?

minism commented 2 years ago

I don't think a default is necessary. An API to do what you suggested along with a documentation snippet should be sufficient to help people quickly get it working, while making it clear that they need to choose which fields are exposed.

I can take a stab at this. Would a function exported by serialization/context_processors.py be a reasonable place to add this?

silviogutierrez commented 2 years ago

I think the cleanest way is just to make register work and not special case context_processors.

Something like this:

from reactivated.registry import register
from django.contrib.auth.context_processors import auth

@register(auth)
class Auth:
   user: ....

Now, you'll notice in TYPE_HINTS we manually say it's the "return" type. But since annotating the return type of functions is so common, @register should be made to work if you pass in a function, it's annotating its return type.

Then create_context_processor_type can just check the PROXIES (which register) populates.

To run tests or setup locally, review setup.sh and the .github/ci.yaml file . You'll need to call start_database too.

Take a stab at it if you want, I can provide more guidance as needed. Let me know and thanks for the offer to help!

devo-wm commented 1 year ago

@silviogutierrez Where in your local application code might you add this @register?

devo-wm commented 1 year ago

further to the above, how would you select the perms key

from reactivated.registry import register
from django.contrib.auth.context_processors import auth

@register(auth)
class Auth:
   user: Pick[User, 'username', ...]
   perms: 

How might we select the perms key in dict return by auth? Perhaps it's actually not wise to send those back but just wondering what might the strategy be.