Fix #5016 - Views mutate user object when using force_request

[alaminopu]

PR will fix issue - #5016

[tomchristie]

I'm a little concerned about the use of deepcopy here. It's pretty much magic, and there's no guarantee that it might not fail in some cases (user can be a custom model, or have been modified by middleware, and might have almost anything stored in state on it)

I'm not sure what the best action is here. Perhaps we should catch exceptions and fall back to simply assigning user, tho that seems problematic too.

Perhaps I'm being overly cautious about the possibility of deepcopy failing here?

[rpkilby]

I've ran into some weird issues recently with deepcopy in regards to the request object. Although it'd be unusual, it's not inconceivable that the request would be somehow accessible from the user via an authenticator or middleware, which deepcopy would pick up. I'm also hesitant about deepcopy here.

Although, if this is happening before any middleware or authenticator machinery, then it's probably fine?

[tomchristie]

One comment for open discussion.

[tomchristie]

On balance I'd say let's accept this, but defer it to the 3.7 release, rather than introduce it in a minor release.

[carltongibson]

@tomchristie We can have this — with #4102 tests are there — but just wondering if a note in the docs saying "You may need to call refresh_from_db on your user if the in-memory state will not be correct" is not a better option?

This was from the issue:

        # At this point in the test code one would expect that user1
        # wouldn't have any 'gender' property, since we never saved the
        # user model in the 'put' method. However, the implementation of 
        # force_authenticate uses the user object that's passed in directly
        # instead of doing `user = copy.deepcopy(user).`

        request2 = self.factory.get('/user')
        force_authenticate(request2, user=user1)
        resp2 = self.view(request2)

But here we know user1 isn't correct. So user1.refresh_from_db() needs to be called before reusing the user.

(What do you think?)

[tomchristie]

@carltongibson I think i'd prefer that, yup - good call. deepcopy is a really fuzzy bit of behaviour to introduce here.

[carltongibson]

OK. I'll make it so.