Add the possibility to configure a serializer for expiry in LoginView

[loicgasser]

Fixes #178

This is what I had in mind.

• We keep the default current behaviour for expiry
• We define a new default Serializer class for exipiry in knox with a simple DateTimeField, which will take the default datetime formatting of the current project.
• We allow the user to define his own serializer for expiry via ExpirySerializer in the settings.

If you agree with these changes, I'll go ahead and adapt the doc.

[loicgasser]

ping @johnraz what do you think?

[johnraz]

Sorry for the delay @loicgasser and thanks for the submission, here are my thoughts.

[johnraz]

First on a purely technical standpoint:

I would try to get rid of this if / else.

get_expiry_serializer_class() should return the default serializer and in this context, ExpirySerializer shouldn't be None, ever.

Something like:

expiry_serializer = ExpirySerializer({'expiry': instance.expiry}, context=self.get_context())
data.update(expiry_serializer.data)

Now on a more architectural standpoint:

I'm wondering if the Serializer isn't a bit too much and introduce indirection and unforeseen use case that would make maintaining backward compatibility harder.

eg: due to the data.update, one could use the ExpirySerializer to add more field to the payload.

Could we reach something simpler with:

data = {
    'expiry': DateTimeField(format=knox_settings.EXPIRY_FORMAT).to_representation(instance.expiry)
    'token': token,
}

and make knox_settings.EXPIRY_FORMAT default to api_settings.DATE_FORMAT ?

[loicgasser]

No problem, thank you for taking the time to review.

get_expiry_serializer_class() should return the default serializer and in this context, ExpirySerializer shouldn't be None, ever.

Yes I think it's better not to either. I didn't know if you wanted to preserve the current behavior where the datetime field is not serialized explicitly in knox.

I'm wondering if the Serializer isn't a bit too much and introduce indirection and unforeseen use case that would make maintaining backward compatibility harder.

Yes I was actually wondering the same, but defining only a field seemed a bit unnatural. I would propose EXPIRY_FIELD instead, because expiry refers to the response key, and field the expected DRF class, same concept as USER_SERIALIZER, but I am fine with EXPIRY_FORMAT or DATE_FORMAT if you prefer it.

On an additional note, why not take the same approach as for user. If not defined, simply don't return the field.

[johnraz]

I'd argue that "expiry" should always be a datetime field, I don't quite see the need for having another field type - do you see a use case where you'd want something else than a datetime ?

Edit:

On an additional note, why not take the same approach as for user. If not defined, simply don't return the field.

I don't think the same thing applies, expiry is linked to the token, whereas the user object is really just needed for some specific flows.

[johnraz]

Also, @loicgasser give a look at #171 (comment).
This could be a more generic approach to offer a way to customize the returned payload without cluttering the library with settings.

[loicgasser]

I'd argue that "expiry" should always be a datetime field

Yes, in my case I am returning an integer. I return all dates as UNIX timestamps. If you use momentjs on the front-end to handle dates, that's probably what you would want to have. I think it's a common use case for SPAs.

Also, @loicgasser give a look at #171 (comment).
This could be a more generic approach to offer a way to customize the returned payload without cluttering the library with settings.

yes, both approaches are not mutually exclusive though. As a user of knox, I'd rather configure a field in the settings.

Let me know what is your decision on that and I'll adapt the code accordingly.

[johnraz]

Yes, in my case I am returning an integer. I return all dates as UNIX timestamps. If you use momentjs on the front-end to handle dates, that's probably what you would want to have. I think it's a common use case for SPAs.

I understand the need but I'm not so sure it's a common use case, it's really hard to tell.
You could also just use the format "%s" to represent the date, return a string in the payload and then deal with the type conversion client side.

yes, both approaches are not mutually exclusive though.

I totally agree

As a user of knox, I'd rather configure a field in the settings.

This would indeed be very flexible but I'm not sure we could instantiate a field in the settings like that:

• is DRF loaded yet?
• is a field instantiated globally, safe to reuse between calls?

If the above works, let's go for it, if not let's fall back to the format string.

I'd also like input from other collaborators of the project (@belugame , @sphrak , @James1345 )

[belugame]

@johnraz I like your idea of having it solved with the same concept as e.g. Django's SHORT_DATETIME_FORMAT. Keep it small and simple. 👍

PS: If you use moment.js on the frontend then you should already be reasonably flexible with the date format given by the backend, I would think.

[loicgasser]

Here is my proposal.

Per default the new setting EXPIRY_DATETIME_FORMAT is formatted using the DATETIME_FORMAT of Django Rest Framework with a DateTimeField. It doesn't solve my use case but I can understand you don't want to support this out of the box.

Additionally, we implement @johnraz proposal where we separate the formatting of the response by adding get_post_response as an additional method on the login view which can be easily overridden to return any custom response to support virtually all use cases.

[johnraz]

@loicgasser let's do that and let's open a new PR for #171 (I'll add some more on the conversation there because I thought about some changes).

edit:
Answered in a hurry, forgot to thank you for being so understanding as to where the feature should go 👍