Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Makes it easy to change AuthToken model class when overriding built-in O... #2151

Closed
wants to merge 1 commit into from

Conversation

ur001
Copy link

@ur001 ur001 commented Nov 27, 2014

Makes it easy to change AuthToken model class when overriding built-in ObtainAuthToken view.
ObtainAuthToken view has a model attribute, but it uses a Token class whatever in post() method.

@maryokhin
Copy link
Contributor

I thought .model was deprecated in #1784. Why does Token view even have it?

@tomchristie
Copy link
Member

Looks like some tweaks needed if this view (eg let's also use .is_valid(raise_exception=True)

@maryokhin
Copy link
Contributor

You mean something simple like this?

class ObtainAuthToken(APIView):
    throttle_classes = ()
    permission_classes = ()
    parser_classes = (parsers.FormParser, parsers.MultiPartParser, parsers.JSONParser,)
    renderer_classes = (renderers.JSONRenderer,)
    serializer_class = AuthTokenSerializer

    def post(self, request):
        serializer = self.serializer_class(data=request.data)
        serializer.is_valid(raise_exception=True)
        user = serializer.validated_data['user']
        token, created = Token.objects.get_or_create(user=user)
        return Response({'token': token.key})


obtain_auth_token = ObtainAuthToken.as_view()

I don't think there's much we can do with the hardcoded Token model, as the serializer/view don't know how to create an instance. I read in #399 that you didn't want to use CreateAPIView. Still the same thoughts as of today?

@tomchristie
Copy link
Member

You mean something simple like this?

Exactly, yes.

I would drop the serializer_class attribute and instead just use AuthTokenSerializer in the post method directly. Anyone wanting to customize the view behavior should just do so explicitly.

you didn't want to use CreateAPIView.

Probably simplest as it is, yup.

carltongibson pushed a commit that referenced this pull request Nov 28, 2014
Update token auth view. Closes #2151.
@carltongibson carltongibson reopened this Nov 28, 2014
@carltongibson
Copy link
Collaborator

This was closed automatically by merging ... argh — I was going to ask you to review @tomchristie (since I wasn't sure they were identical) — but I see you have.

@tomchristie
Copy link
Member

@ur001 Contrary to some python design you'll see we actually choose to favor slightly more coarse grained API in some cases. The "hooks for everything" approach doesn't recognize that there's a trade off in increasing the surface area of the API.

In this case if you need to alter the behavior just override the view explicitly. It's still drop-dead simple, but there's less API for us to document and support, and your view behavior will actually be more obvious and maintainable by not relying on API hooks and implicit behavior.

Hope that gives enough context to explain the design decision. It runs contrary to what you'll tend to see, but I think it actually makes for better design.

(Yup, thanks @carltongibson!)

@ur001
Copy link
Author

ur001 commented Nov 28, 2014

@tomchristie ok, I agree with your approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants