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

UUID Cast Error #2147

Closed
hpopp opened this issue Aug 1, 2017 · 8 comments
Closed

UUID Cast Error #2147

hpopp opened this issue Aug 1, 2017 · 8 comments

Comments

@hpopp
Copy link

hpopp commented Aug 1, 2017

Took me a while to track down the change in #2056, butUUID.cast/1 fails rather non-obviously if UUIDs don't conform exactly to the UUIDv4 spec. I have some legacy data where integer primary keys had been migrated to UUIDs by padding with zeros (e.g. "00000000-0000-0000-0000-000000000034")

Resulting error:

** (ArgumentError) cannot load `<<0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 34>>` as type :binary_id for :id in schema Repo.User          
    (ecto) lib/ecto/schema.ex:1418: Ecto.Schema.load!/5                                                                                      
    (ecto) lib/ecto/schema.ex:1399: Ecto.Schema.__load__/5                                  
    (ecto) lib/ecto/schema.ex:1371: Ecto.Schema.__load__/6                             
    (ecto) lib/ecto/adapters/sql.ex:582: anonymous fn/4 in Ecto.Adapters.SQL.process_row/4
    (elixir) lib/enum.ex:1357: Enum."-map_reduce/3-lists^mapfoldl/2-0-"/3                                                                    
    (ecto) lib/ecto/adapters/sql.ex:579: Ecto.Adapters.SQL.process_row/4                                                                     
    (postgrex) lib/postgrex/query.ex:77: DBConnection.Query.Postgrex.Query.decode_map/3                                                      
    (postgrex) lib/postgrex/query.ex:64: DBConnection.Query.Postgrex.Query.decode/3 

Unsure whether the validation should be reversed or just better noted in the docs.

Environment

  • Elixir version: 1.5.0
  • Database and version: Postgres 9.4
  • Ecto version (mix deps): 2.1.5
  • Database adapter and version (mix deps): postgrex 0.13.0
  • Operating system: Debian
@michalmuskala
Copy link
Member

Does Postgres accept those as valid UUIDs?

@hpopp
Copy link
Author

hpopp commented Aug 1, 2017

Accepts them just fine. I think the uuid type was meant to be as generic.

michalmuskala added a commit that referenced this issue Aug 1, 2017
@michalmuskala
Copy link
Member

We relaxed the validations in the 2.1 branch and released 2.1.6 with the fix.

The validations will be kept on the 2.2 branch and Ecto.UUID will only accept UUIDs conforming to the specification in https://tools.ietf.org/html/rfc4122

@potatosalad
Copy link

@michalmuskala As far as I can tell, RFC 4122 only uses the version and variant for the creation of Time-Based, Name-Based, and Random Number UUIDs.

In fact, Section 3 states:

Validation mechanism:

Apart from determining whether the timestamp portion of the UUID
is in the future and therefore not yet assignable, there is no
mechanism for determining whether a UUID is 'valid'.

The example implementation at the bottom of the RFC does not validate UUIDs.

ITU-T Rec. X.667 also states in Section 10:

Apart from determining if the variant bits are set correctly, and that the Time value used in a time-based UUID is in the future (and therefore not yet assignable), there is no mechanism for determining if a UUID is valid in any real sense, as all possible values can otherwise occur.

Other standards, like ISO/IEC 9834-8, also focus primarily on the generation of UUIDs.

Neither PostgreSQL, MySQL, MongoDB, Oracle, nor SQL Server validate UUIDs in any way beyond checking that they represent 128-bits, 16-bytes, or are composed of 32-bytes of hexadecimal characters (36 if you include the hyphens).

The current (Ecto 2.2) UUID validation also rejects the Nil UUID as specified in Section 4.1.7 of RFC 4122:

iex> Ecto.UUID.cast(<<0::128>>)
:error

I use ULID for UUIDs that are lexicographically sortable, which is not currently compatible with Ecto 2.2.

So, all of that said: I do not think there is any basis that Ecto should be validating UUIDs in its current form (ie. validating version and variant).

@josevalim josevalim reopened this Aug 9, 2017
@potatosalad
Copy link

Also, I'd be happy to submit a pull request, but wanted to see if anyone had a differing opinion first 😀

@josevalim
Copy link
Member

Thank you @potatosalad! I have reverted those changes and pushed to master!

@potatosalad
Copy link

Huzzah! My weird ids work again! Thanks, @josevalim.

@michalmuskala
Copy link
Member

I guess I can't say anything other than: "I'm convinced" 😃

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

No branches or pull requests

4 participants