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 binary type #32

Merged
merged 2 commits into from
Jul 18, 2024
Merged

allow binary type #32

merged 2 commits into from
Jul 18, 2024

Conversation

dnsbty
Copy link
Contributor

@dnsbty dnsbty commented Jul 17, 2024

Thanks for taking the time to create this library! I'm using it in a project I'm working on, and I noticed the behavior reported in #31 where calling TypeID.new/1 would generate K-sorted IDs, but letting Ecto autogenerate them would not. Because of that I dug into EctoSQL to understand it's behavior a little better, and came across this:

https://github.com/elixir-ecto/ecto_sql/blob/b1f7386c6465d78e1e09691ed11fa4ad5e854fcd/lib/ecto/adapters/sql.ex#L187

Basically if a parameterized type uses :binary_id as the underlying type, EctoSQL will autogenerate it using its own autogenerate implementation rather than the autogenerate method of the custom type.

I debated a little bit about whether to try changing EctoSQL to allow autogenerating a custom type with binary_id underlying it, but it seems like :binary_id is a special construct meant just for that library's UUID creation, and really what TypeID cares about is what the underlying storage type is. And since that's the case, I fail to see any reason why :binary_id should be used over :binary. So this pull request updates the library to allow :binary as well, and when I switched all my IDs from :binary_id to :binary they started autogenerating in a K-sortable way again.

I tried to keep this pull request as simple as possible, but I would recommend that the :binary_id type be removed completely in favor of :binary. Because the library is still pre-v1.0 I think it would be acceptable to make that change with a minor version bump. Otherwise a deprecation warning could be added for :binary_id usage in TypeID.Ecto.validate_opts!/1 and the functionality could be removed later. I'm happy to make that change in this pull request as well if you would like, but I didn't think it was my place to make that decision without input.

@sloanelybutsurely
Copy link
Owner

thank you for investigating this! i'll be able to review this later today

Copy link
Owner

@sloanelybutsurely sloanelybutsurely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks great! are you able to change this to replace :binary_id with :binary completely? because :binary_id doesn't work (in that it does not generate true k-sortable IDs as advertised) i'd prefer a breaking change to remove the erroneous behavior. thanks again!

@dnsbty
Copy link
Contributor Author

dnsbty commented Jul 18, 2024

@sloanelybutsurely I think that makes complete sense. I've just removed :binary_id completely.

Copy link
Owner

@sloanelybutsurely sloanelybutsurely left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you so much @dnsbty!

@sloanelybutsurely sloanelybutsurely enabled auto-merge (squash) July 18, 2024 14:13
@sloanelybutsurely sloanelybutsurely merged commit c74d433 into sloanelybutsurely:main Jul 18, 2024
2 checks passed
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.

2 participants