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

Add support for non-integer PKs #9

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jerivas
Copy link
Contributor

@jerivas jerivas commented Feb 24, 2021

This PR adds support to broadcast updates for models that don't use integer PKs, for example UUID or HashID. The biggest change is converting pks_in_queryset to a set of strings (instead of integers). This will support any type that can be reliably cast to a string.

@davish
Copy link
Member

davish commented Feb 25, 2021

The code looks good to me, and the tests pass, but I'm wondering about how querysets and the ORM handle the different types. Does django not care if you pass in a string or an int for a CharField / AutoField? Will it coerce to the proper type before firing off the SQL query? I'll look into the docs a bit more but wanted to ask in case you had experience with this.

@jerivas
Copy link
Contributor Author

jerivas commented Feb 25, 2021

I would suspect it works fine since the original tests are passing and their pks are being cast to strings as well

@jerivas
Copy link
Contributor Author

jerivas commented Mar 2, 2021

@davish I just verified it and it works: Django will find instances by their PK even if it's passed as a string and the actual value is an int

Previously modifying other instances in the queryset would trigger messages for all "retrieve" subscriptions
@jerivas
Copy link
Contributor Author

jerivas commented Mar 10, 2021

@davish I fixed a bug where retrieve messages were being sent for unrelated model instances (also added a test case)

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

Successfully merging this pull request may close these issues.

2 participants