sloanelybutsurely / typeid-elixir

Elixir implementation of TypeIDs: type-safe, K-sortable, and globally unique identifiers inspired by Stripe IDs
MIT License
58 stars 8 forks source link

allow binary type #32

Closed dnsbty closed 3 months ago

dnsbty commented 3 months ago

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 commented 3 months ago

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

dnsbty commented 3 months ago

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