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

Replace HexIntegerField with a CharField #358

Open
jleclanche opened this issue Feb 20, 2017 · 15 comments · May be fixed by #386
Open

Replace HexIntegerField with a CharField #358

jleclanche opened this issue Feb 20, 2017 · 15 comments · May be fixed by #386
Milestone

Comments

@jleclanche
Copy link
Member

HexIntegerField has been the source of many issues for some database backends. It's not really worth the trouble, especially as it makes it hard to search the field by hex and impossible by partial hex.

Replacing this needs a design plan that will account for migrating existing DBs.

Open for the taking.

@jleclanche jleclanche added this to the 2.0 milestone Feb 20, 2017
@matthewh
Copy link
Collaborator

matthewh commented Apr 3, 2017

@jleclanche I agree - this is causing me some headache right now. Would you accept this also in 1.5?

@jleclanche
Copy link
Member Author

Probably not; I think this is a really backwards incompatible item. I could see it in 1.6 if there is one.

@matthewh
Copy link
Collaborator

matthewh commented Apr 3, 2017

I tend to look at it as relaxing the requirement. Any old value would still be accepted but would now be a string. I'm not keen on the idea of going through all this work and still needing to maintain a my own branch for production.

@jleclanche
Copy link
Member Author

It requires a data migration though which I'm not a fan of working into 1.5.

@matthewh
Copy link
Collaborator

matthewh commented Apr 3, 2017

But you'd accept making it a UUIDField like APNS and WNS for 2.0?

@jleclanche
Copy link
Member Author

Sure.

@jleclanche
Copy link
Member Author

Actually, what do you mean exactly? UUIDs are 128bit, the hexints are 64.

@matthewh
Copy link
Collaborator

matthewh commented Apr 4, 2017

I was leaning towards converting any non-UUID value into a UUID using UUID5. The benefit here is all platform device_ids are now uuids. It would be transparent when adding device_ids to the system as we could convert any non-UUID into a uuid. We can also apply the same logic when filtering. This would allow application developers to use alternative values to the expected device_ids in situations where they have a more reliable way to determine the device id than indicated by the vendor.

@jleclanche
Copy link
Member Author

Why uuid5?

@matthewh
Copy link
Collaborator

matthewh commented Apr 4, 2017

UUID5 is based on a SHA-1 hash of a namespace identifier, a UUID we can specify, and a string, the device_id. This basically lets us create hashes using uuids for sources that are not uuids.

Alternatively, consider switching device_id to just a CharField across the board and let application developers treat it however they deem necessary.

@jleclanche
Copy link
Member Author

I'd rather switch to a uuidfield which is zero-padded on the leftmost 64-bit.

@matthewh
Copy link
Collaborator

matthewh commented Apr 4, 2017

I'd be willing to try that but I wonder if it would validate.

@matthewh
Copy link
Collaborator

matthewh commented Apr 8, 2017

Switching to a UUIDField that is zero-padded is pretty trivial. I'll get a PR ready.

>>> import uuid
>>> uuid.UUID(int=123)
UUID('00000000-0000-0000-0000-00000000007b')

@matthewh
Copy link
Collaborator

matthewh commented Apr 8, 2017

@jleclanche It also appears that the HexadecimalField is unused. Safe to delete fields.py?

@jleclanche
Copy link
Member Author

jleclanche commented Apr 8, 2017 via email

@matthewh matthewh linked a pull request Apr 17, 2017 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants