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

Allow devices to use custom primary keys #29

Open
wants to merge 1 commit into
base: master
from

Conversation

@nickcatal
Copy link

nickcatal commented Feb 12, 2020

There may be instances where the primary key of a device is not an integer (such as when an app does not want an auto-incrementing integer on their devices models and instead want to use a UUID.)

Django will allow you to query using pk= instead of id= in filters, so switching to that is pretty easy.

There may be instances where the primary key of a device is not an
integer (such as when an app does not want an auto-incrementing integer
on their devices models and instead want to use a UUID.)

Django will allow you to query using `pk=` instead of `id=` in filters,
so switching to that is pretty easy.
@psagers

This comment has been minimized.

Copy link
Member

psagers commented Feb 13, 2020

That sounds reasonable in principle. Is this an urgent need or a speculative improvement? A few questions come to mind.

persistent_id is currently split on '/' from the right; can you guarantee that a custom primary key value will never contain a slash character?

If the documentation were updated with an explicit requirement that all concrete device models use Django's default integral primary key, would that make certain edge cases impossible or merely inconvenient?

If this is worth doing, perhaps it's worth doing right. Is there a more robust way to encode persistent IDs? I've also long wondered if the abstract device model shouldn't have just had a UUID field for this purpose.

@nickcatal

This comment has been minimized.

Copy link
Author

nickcatal commented Feb 13, 2020

@psagers it's a priority for us (we're going into production with the change) but I see where you're coming from. We try to use uuids since using an auto-incrementing primary key for devices would risk unnecessarily disclosing the total size of our userbase and the level of adoption of 2 factor auth.

I noticed the issue with persistent_id and was thinking through that, and yeah there is no way to prevent a primary key that has a slash from causing havoc. You could left split it. The risk of someone creating a model label with a slash is probably significantly smaller than the risk of someone creating a primary key with a slash. Do you think left splitting addresses your concern.

@psagers

This comment has been minimized.

Copy link
Member

psagers commented Feb 13, 2020

Seems like it would be impossible to create either an app or model name with a slash, so that should be fine.

I gather you've found that UUIDs coerce to and from strings with no issues. We'd want to update the documentation to indicate that non-integral primary keys are supported, but only if they support this string round-trip. And of course a test to at least make sure that the UUID scenario never gets inadvertently broken in the future.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.