jetify-com
Support parsing and detecting empty values
Hello, very interesting package. I noticed there is no IsZero() method, which I expected, and then I discovered that parsing an empty string returned a random ID which means I can't use it for my use case: https://go.dev/play/p/ZFuKTgUop1O
@hkeide thanks for pointing this out. I've created a PR that fixes the empty string bug, and adds an IsZero method: https://github.com/jetify-com/opensource/pull/352
Once we merge that, the next version of the library should have the functionality you need.
@hkeide Note that as I'm implementing it, IsZero returns true if the suffix is zero regardless of the prefix. In other words, all of these values would return IsZero() == true:
prefix_00000000000000000000000000prefix_two_0000000000000000000000000000000000000000000000000000
I think that's the right behavior, but I wanted to double check that that works with the use case you have in mind.
One last thing. I'll point out that before my PR, you can still create a zero ID yourself by using an empty TypeID struct. For example: https://go.dev/play/p/KDqJUPhjve4
Hi, first of all I'm very impressed with your response time here!
I think that your proposed IsZero behavior is correct, since I'm thinking that an ID that is being used for anything useful should never have a zero suffix, so we don't want to return false in that case. One can still detect the prefix using separate logic which seems perfect to me.
About the parsing logic, I haven't thought about wether it should be an error or just return a zero value when parsing an empty string. Having a valid zero-value is very idiomatic in Go, and if I have an empty database column I'm not sure I would want that to be an error when unmarshalling from the database, but I can also see the benefit of being strict, and then just having the database column being NULLable if it needs to be optional, and dealing with that in a different layer. 🤔
Maybe you have some thoughts on this final point since you probably have more experience with this?
The database question is interesting – I don't know if there's an obvious "right way".
While in some ways it's nice to treat the empty string as zero, it also causes other problems. A zero typeid needs to map to the zero UUID when you remove it's prefix. The zero UUID is 00000000-0000-0000-0000-000000000000 which, based on our encoding, maps to the 00000000000000000000000000 suffix.
If we allow the empty string as a valid representation of the zero typeid, all of a sudden you have two possible string representations for the same id: "" and "00000000000000000000000000". So while it's convenient, it also means your data is not "normalized" – if you have a string column that is supposed to encode typeids, you now have to search for both values (and possibly NULL too if your column is NULLABLE).
Our Scan implementation currently treats the empty string as nil (same as if the column was NULLABLE and the value is NULL) instead of the zero id. See https://github.com/jetify-com/typeid-go/blob/main/sql.go#L17
What we do internally for our database modeling is that we make our typeid columns NOT NULLABLE (unless it's a case where the ID is optional), AND we add constraints to the column ensuring the value is always parses as a valid typeid (including making sure it's not the empty string). The constraints we add live in: https://github.com/jetify-com/typeid-sql – although I've been wanting to try the native postgres extension that @blitss recently contributed: https://github.com/blitss/typeid-postgres
Just for some background, in the extension you can't pass the invalid TypeID representation, e.g. with an empty string. If you convert invalid typeid string to TypeID type, like 'prefix_'::typeid or ''::typeid that will return an error. A correct TypeID must contain an ID. The column can also be null, but that's up to whether you make a postgres field nullable or not
Hey
I'm just popping out to drop a message.
Guys, I'm impressed by the quality of the discussion, and how respectfully and open minded you guys are.
A random Gopher, watching the repository
@loreto I finally had time to look at the code in the PR and play around more with this library, and I think it looks really good.
I now think that not accepting the empty string at all is the correct choice, like you said above. I think this is the correct choice both for DB serialization and other purposes like JSON serialization since the empty string is very likely a bug.
So I guess this issue can be closed.