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

Added an optional USER_SESSIONS_USER_TO_FIELD setting #18

Closed
wants to merge 2 commits into from

Conversation

ydaniv
Copy link

@ydaniv ydaniv commented Sep 15, 2014

Allowing configuration of Session.user.to_field.

My use case: I have a field uuid on my Account model which is the actual field I'd like to reference. I can't make that field primary yet since it will break other parts of Django or other packages that expect Account.pk to be an integer.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 9135a5e on COMMOLOGIC:master into 105ec5c on Bouke:master.

@ydaniv
Copy link
Author

ydaniv commented Sep 15, 2014

Running a South automatic schemamigration claims nothing has changed.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling ab09c9b on COMMOLOGIC:master into 105ec5c on Bouke:master.

@ydaniv
Copy link
Author

ydaniv commented Sep 15, 2014

Hmmm... South lied ):
Probably because I'm getting the field's name dynamically from settings.
Fixing...

@ydaniv
Copy link
Author

ydaniv commented Sep 15, 2014

This gets tricky in case the field's type is changed ):
Closing this and going back to the lab.
Will be happy to know if this is at all appropriate and likely to be considered for merging.

@ydaniv ydaniv closed this Sep 15, 2014
@Bouke
Copy link
Collaborator

Bouke commented Sep 16, 2014

Yes, changing the type down the project will probably have a large impact. So why do you wan't to continue using another field, which is not the primary key, while keeping the actual the primary key in place? You could use the actual primary key as well.

@ydaniv
Copy link
Author

ydaniv commented Sep 16, 2014

The problem is that some packages, even Django itself, still consider the User's primary key to be an integer.
But I'm trying now to go down that path once again and see if I can pull it off with latest versions.

So now the initial migration of user_sessions is problematic since it states that the User's primary key is a django.db.models.fields.AutoField which translates to an integer type.

Seems that overriding it, e.g. with uuidfield.fields.UUIDField, does the trick.

Is there a way we can at least make that part dynamic?

@Bouke
Copy link
Collaborator

Bouke commented Sep 16, 2014

You'd probably have to look into making this part dynamic. In order me to merge this pull request, your changes should not include extra configuration settings. I think it should work when you set the User's primary key to the UUIDField and than the initial migration should pick-up on this. So that would require inspecting the type of the primary key.

For your other problem with Django itself, you could have a look into overriding the migrations that are created. Or patch the models altogether in your code.

@ydaniv
Copy link
Author

ydaniv commented Sep 17, 2014

You'd probably have to look into making this part dynamic

Yes, that is the part I was asking about:

Is there a way we can at least make that part dynamic?

I don't think there's a way making this dynamic, unless it's settings option.

My only way out of this now is patching this in my own fork.

If it's something you're willing to consider, please let me know and I'll work on a new PR.

Thanks a million!

@Bouke
Copy link
Collaborator

Bouke commented Sep 17, 2014

I think it would work in the migrations, if you changed the relevant part to this:

        User._meta.pk.attname: (
            '%s.%s' % (User._meta.pk.__class__.__module__, User._meta.pk.__class__.__name__), [],
            {'primary_key': 'True',
            'db_column': "'%s'" % User._meta.pk.column}
        ),

Can you try to get this to work?

@ydaniv
Copy link
Author

ydaniv commented Sep 18, 2014

I'll give it a try, thanks!

Another thing I came across was that when the sessions backend is attempting to JSON-serialize the session it's breaking because the session key, i.e. the user id, isn't serializable since it's a UUID object instance.

Trying to find now the cleanest way to get it cast to a string/unicode on serialization. Any idea?

@ydaniv
Copy link
Author

ydaniv commented Sep 18, 2014

Looks like it Django isn't ready yet for this, see here. And the serialization issue is all over the web without a reasonable solution in sight.

I have an idea on a different approach which I'll open in a separate thread.

Thanks for all your help!

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.

3 participants