[FOLIO-972] The regex pattern for UUID in schemas does not completely allow mixed-case characters Created: 07/Dec/17 Updated: 12/Jun/20 Resolved: 12/Jun/20 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Bug | Priority: | P3 |
| Reporter: | David Crossley | Assignee: | Unassigned |
| Resolution: | Done | Votes: | 0 |
| Labels: | raml | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | 1 day, 2 hours, 40 minutes | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||||||
| Sprint: | |||||||||||||||||||||
| Development Team: | Core: Platform | ||||||||||||||||||||
| Description |
|
To ensure good API docs i regularly run 'raml-cop'. It ensures that RAMLs and schema are well-configured and it validates the examples. I am finding UUIDs in examples that are not matching their defined regex. Investigation shows that that pattern does not allow fully mixed-case UUIDs. |
| Comments |
| Comment by David Crossley [ 07/Dec/17 ] |
|
Some investigation: Searched Jira for "uuid regex":
Scanned all repositories. There are two unique patterns: A:
^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[1-5][a-fA-F0-9]{3}-[89abAB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}$
That is only used in B:
^[a-f0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$
That is used in the shared raml-util at In mod-circulation mod-circulation-storage and in the raml-util, the examples all use lower-case UUIDs. That regex B does allow mixed-case characters, except for the first portion of the UUID. This is what causes the raml-cop complaints in the examples of those other modules, where they do have mixed-case UUIDs, and it chokes on the upper-case in the first portion. Should pattern B be amended? |
| Comment by Marc Johnson [ 07/Dec/17 ] |
|
David Crossley Yeah, I think the B style regular expression should be amended to allow uppercase letters in the first section (I don't recall if there was a reason not to, so it was likely an omission) and the A style use replaced by the amended B style. I did a little digging this morning, my interpretation (have quoted what I think is the relevant bit below) of the specification is that we should generate UUID representations with lowercase letters, yet accept representations with either case (and treat them equivalently I think?) which fits with allowing them in the regular expression.
Aside: can you point me at some of the examples which fail raml-cop, I just ran it on mod-inventory, mod-inventory-storage, mod-circulation and mod-circulation storage and it validated. Do you run it with different parameters? I wonder if the scripts miss some interfaces. |
| Comment by Adam Dickmeiss [ 07/Dec/17 ] |
|
Good catch David Crossley!! |
| Comment by David Crossley [ 08/Dec/17 ] |
|
Marc, all of your examples do pass because they are all lower-case. Try twiddling request.json "id" first segment to see a failure. No, there are no other parameters to raml-cop. |
| Comment by David Crossley [ 08/Dec/17 ] |
|
That is my understanding of UUIDs too. So that (amended) pattern B will ensure a UUID-like string, whereas pattern A ensures a real one. I will go ahead and fix that pattern B in the schemas of those various repositories. It also involves updating the raml-util in each, with the shared metadata.schema file. Marc, for your repositories, please update that when you are ready. |
| Comment by David Crossley [ 11/Dec/17 ] |
|
Fixed that pattern B in raml/schemas/metadata.schema and awaiting merge of its raml/pull/44 And fixed in mod-circulation mod-circulation-storage mod-finance mod-vendors. Awaiting merge of "dev" branch in mod-orders. |
| Comment by David Crossley [ 11/Dec/17 ] |
|
Marc Johnson, not yet touched the use of pattern A in mod-circulation/ramls/circulation.raml |
| Comment by David Crossley [ 11/Dec/17 ] |
|
While investigating this i found that many repositories still just use "string" and no pattern to restrict for IDs. I gather from the linked issues that this is a gradual transition. |
| Comment by Marc Johnson [ 11/Dec/17 ] |
|
David Crossley Yeah, it is my understanding that retro-fitting UUID validation to existing ID properties is intended to be a gradual thing, given that it is a compatibility breaking change to an interface (even though we should be using valid UUIDs in those properties) |
| Comment by Jakub Skoczen [ 12/Jun/20 ] |
|
David Crossley assuming this is done. |