Add uuid type with primary key coercion #32
Conversation
Adds a check to normalize.expandPK on uuid type primary keys, and coerces them to a UUID-patterned string. Adds normalize.expandPK to deferred so that chained `where` calls also coerce the criteria.
Adds a test in the finder query test for checking that UUIDs get defered. The PK coercion behavior doesn't seem to be tested at this level, but this seems like the most appropriate place.
| coercePK = function(pk) { | ||
| var pattern = /[a-f0-9]{8}-[a-f0-9]{4}-4[a-f0-9]{3}-[89aAbB][a-f0-9]{3}-[a-f0-9]{12}/i | ||
| var matches = pattern.exec(pk); | ||
| return matches && matches[0]; |
There was a problem hiding this comment.
This will return null if the input doesn't match the uuid pattern. Do we care? Or should this throw an error (TypeError maybe?)
There was a problem hiding this comment.
I'd rather throw an error, though let's check to make sure the callers wrap this in a try/catch block.
There was a problem hiding this comment.
Er, oh. I believe we'll still pass this to lusitania at some point?
There was a problem hiding this comment.
Maybe we should do the UUID validation there https://github.com/shyp/lusitania
There was a problem hiding this comment.
I think this is the right place for this. From what I can tell, the coercion/normalization behavior is supposed to be independent of the higher level model-level validation, because it's on the query criteria, and not the model instance, and gets used on reads (AFAICT there's no validation on reads). "foo_abc123..." isn't invalid per-se, it's like passing "123" to an integer primary key.
As for errors, I don't think you can have a null primary key in PG, but "this should never happen" is a good reason to throw. I believe it'll behave like a WLError being thrown when something goes wrong with the query--otherwise I don't think there are any other errors being thrown in the places where these are being called (e.g. here). I can look a little further into that and see what's normal/expected in the caller.
There was a problem hiding this comment.
Ok SGTM. any chance we can add a small doc block explaining what's going on?
There was a problem hiding this comment.
Yep. Should it be doc'd anywhere else you know of?
There was a problem hiding this comment.
i mean there's a waterline-docs project but we don't really use that :( maybe api/db or something, I don't know
There was a problem hiding this comment.
Ah, yeah I can put a note in the API about how to use this when I get the trackingevents stuff up.
|
Some of our tables store references to other tables primary keys, e.g. pickups.userId - do we have a plan for those? or later? |
|
I think this will only be an issue if we were querying for one model by a prefixed foreign key. Part of the plan is migrating associated keys (not least of which because removing the prefixes breaks any foreign key constraints), so for joins and internal queries, it should be fine. So in the example of pickups.userId, you'd have to be looking up pickups where the userId is I didn't encounter that scenario in looking at |
| .where({ id: 'foo_b0aec906-81d1-401b-8622-2b01036793d5' }) | ||
| .exec(function(err, results) { | ||
| assert(!err); | ||
| assert(results[0].where.id == 'b0aec906-81d1-401b-8622-2b01036793d5'); |
Changes primary key type coercion on UUIDs to throw a TypeError when attempting to coerce a value that is not a UUID.
|
Please ping me when this is ready for re-review as I'm not checking this project regularly :( |
This removes the uuid-specific test for testing normalization of primary keys in deferred queries. This behavior can more simply be checked with the pre-existing test setup by simply checking that string representations of integers are properly coerced to their integer form.
Change normalize.expandPK to only coerce the uuid when the underlying schema is reported as UUID as well.
|
Alright, this is ready for another review. In addition to addressing feedback, I tweaked the coercion rule slightly to also look at |
|
Looking! |
|
|
||
| // If the the data type is uuid (both according to the model definition | ||
| // as well as the database schema) attempt to coerce the value to a | ||
| // properly formatted uuid, stripping any extreneous characters from it. |
Fix typo and pull out UUID regex.
|
LGTM |
|
Thanks for the review! |
https://www.pivotaltracker.com/story/show/117363295
This patch adds coercion behavior when a model attribute is specified as a
uuidtype and designated a primary key. This is similar to existing behavior in waterline where numbers and strings are coerced, and this patch extends this behavior. In other words, you can pass{id: 'foo_0b6c28e0-a117-4a9e-9a0d-60f0992edbee'}to a.whereand waterline will strip the"foo_"from the id.Note that this will not coerce uuid types that aren't primary keys. There's no mechanism in waterline for this (that I know of), so it'd be a larger change, both behavior and code-wise.