Add UUID support

[kalinon]

added basic UUID object support. This still correctly reads/writes to a binary(36) field.

[bcardiff]

@kalinon I've never user UUID in mysql so I might be lacking some information. It seems that UUID is not a type in mysql but they are stored in either binary or string fields.

If there are two options, I am missing how the driver would know how to send a UUID from crystal without additional information.

On another side, we would need some specs ;-)

[jhass]

Actually if we pick one of the two as a default I would strongly vote for binary instead of string representation, given its much more efficient.

[kalinon]

You are correct, they started to add more UUID support in MySQL 8. https://dev.mysql.com/doc/refman/8.0/en/miscellaneous-functions.html Which is really stored in a binary(36) field.

I think binary passing would be more efficient, however i don't understand the mysql/db framework enough to implement it. As for specs, i can attempt to add some.

[bcardiff]

If it's not a proper type, I think this is another instance that should be solved by a converter module (that still does not exist :-P). But we have other types that are pushing in that direction I think.

The specs that use sample_value aim to write/read values to ensure their behavior is sound regarding text protocol, binary protocol, storing them in tables, passing them as arguments, etc.

Currently applying conversion when #read(UUID) is used is easy, because the underlying value will be either a string or a slice, hence the transformation to do can be determined. The challenge is in the other direction, when the parameter to send is an UUID and we can't know if the sql statement is expecting a slice or a string.

[kalinon]

@bcardiff rounded back around to this today. We can't seem to use sample_value, because, as you said, its not a defined type and unless it was used with decl_type i don't think the pieces are generated.

Perhaps we just leave it to the user to do the translation.

[bcardiff]

Ok, so we can aim to make rs.read(UUID) work when the underlying is String or Slice directly I guess.

But making UUID work as a parameter will imply choosing the representation on our side. Right? If we can support both representations with the same code then we can move forward. But if the code will support only one of the column type, then, I would prefer to leave it to the user. A runtime exception could be raised at most. (Other drivers might support UUID so doing a compile-time exception is not an option.)

These scenarios should have a spec definitely to have a statement which support is expected. If there is nothing that can be reused from sample_value, at least a custom table and query should be used.

---

• In your scenario, you are only sending UUID but not reading them right?
• Feel free to close the PR if this has fadeout out of your interest in the sight of the pros/cons. But also feel more to move forward :-)

[kalinon]

@bcardiff thanks for the summary of next steps. Let me run with that.

[kalinon]

Good news is I've made some progress. I've been able to write to the column correctly using Bytes. However, reading seems to be an issue because it returns the column type as String instead of Bytes.

MySql::ResultSet#read returned a String. A Slice(UInt8) was expected. (Exception)

Debugging the column return shows that it does come back as string, but with the binary bitmask set.

catalog, schema, table, org_table, name, org_name, character_set, column_length, column_type, flags, decimal
def, crystal_mysql_test, uuid_test, uuid_test, uuid, uuid, 63, 16, 254, 4225, 0

I'm guessing the behavior of a read of a binary be to passed back as a String is intentional.

[bcardiff]

In https://github.com/crystal-lang/crystal-mysql/blob/master/src/mysql/result_set.cr#L83 and https://github.com/crystal-lang/crystal-mysql/blob/master/src/mysql/text_result_set.cr#L86 you will find that binary encoded strings should be returned as Slice(UInt8).

Maybe there is a val.is_a?(String) && @columns[col].character_set == 63 missing to convert the string as a slice... it's odd to have to convert it back.

Maybe the conversion instead of just having the type should have the ColumnSpec as input instead of those ugly ifs.

Those checks were added in #9, so as long as those specs pass we should be fine :-) specs FTW!

[kalinon]

@bcardiff just pushed updated changes. Binary read/write works when type is a binary column. If it is a string column, it does not. I think this should be desired behavior if we go with @jhass 's suggestion.

I am quite surprised that the flags are not accessible via the read/write method. if they were, we could handle the correct conversion.

[bcardiff]

@kalinon I tested and refactored the code to remove duplication, and take advantage of sample_value to stress corner cases.

I think is best to keep a unified behavior accross mysql version. This means supporting only UUID stored as binary. If the user wants to store them as String then it's up to them to do the conversion (it might have a - or not, for example). I would not want to assume a textual representation and leave that to the user.

The refactor push me forward to isolate the null_bitmap, which was nice. And the nilable UUID case was missing. The UUID | Bool seems to not be needed.

You can find the outcome in https://github.com/bcardiff/crystal-mysql/tree/uuid-review . If that works on your codebase, we can add those commits in this PR.

[kalinon]

@bcardiff awesome! yeah i was looking at yesterday, ill take a look at your branch today!

[kalinon]

@bcardiff yep! this works great. Thanks for removing the unnecessary pieces. I also agree that we should go forward with supporting UUID stored as binary only. in mysql 8 this is the direction they took.

Ive merged your changes into my branch.

[bcardiff]

I added one more refactor to avoid some allocations. As soon as CI is happy I'll merge this.