[Fix] Update UUID regex to be case insensitive#27
[Fix] Update UUID regex to be case insensitive#27lindyhopchris merged 3 commits intolaravel-json-api:developfrom
Conversation
lindyhopchris
left a comment
There was a problem hiding this comment.
Hey. Thanks the PR. I'm unsure about this though. The fact your database layer only accepts upper case is a database concern.
The regex here is for the id as it appears in the JSON:API, i.e. in the URL plus in the id field of the JSON:API resource. The regex is already case-sensitive, so will allow a UUID in uppercase. Also, the regex here is intentionally a copy and paste of the same regex that Laravel's uuid() route helper uses.
If you want the URL/resource ID to always be uppercase, then you'd need to use a regex that only allowed uppercase, which can be done via matchAs($pattern)->matchCase(). If we wanted to support that via a helper method, we could add a method called upperCaseUuid() that does that.
That I think would be a better approach. What do you think?
|
Yes, it's a database concern that SQL Server uses and accepts upper case UUIDs but the change is required to support SQL Server as RDBMS because currently, it won't work. Also, like in GraphQL, ID values should be treated as opaque without any assumptions. The previous regex is case sensitive but only allows lower case a-f characters, not upper case A-F so upper case UUIDs are not allowed and doesn't work at all. I don't want the UUID to be always in upper case, only add support for SQL Server to the package by default. Therefore, adding more methods has no value. |
|
In addition, Laravel also uses the new regex: |
|
D'oh - of course it was case sensitive before! Thanks for pointing that out! Thanks also for the links to the Laravel regex. I originally copy and pasted this regex from the Laravel routing regex, so clearly they have tweaked it. The intent was to match their regex, so will merge and tag this. I'll squash merge it to tidy up the commit history. Thanks again for the PR rather than just opening an issue 🎉 |
|
Tagged as |
Adds support for MS SQL Server which uses upper case UUIDs and doesn't accept lower case UUIDs.