Open petertseng opened 6 years ago
ferhatelmas
commented
6 years ago @petertseng I am happy it's brought up because I had seen it while doing https://github.com/exercism/go/pull/894 but waited for input from others since I was away for sometime and I might be missing something.
Also, created this issue in UUID dependency: https://github.com/mattetti/uuid/issues/2
nywilken
commented
6 years ago @petertseng @ferhatelmas the concept of a UUID is fairly new, at least the support of it within Configlet. In fact, the lint command only checks for a non-empty UUID and uniqueness across exercises and tracks. Seeing as configlet is the first line of defense for any track changes, I think validating UUIDs should be part of Configlet. But I will defer to anyone on the team who may have more insight on UUID usage and validation across Nextercism.
NobbZ
commented
6 years ago I think formal validity of an UUID should be checked before even asking upstream for duplicates… Of course the API server should check again ;) We know this already from web forms ;)
Since I already copied from wikipedia, a the canonical representation is written with 32 hexadecimal digits, grouped with a rythm of 8-4-4-4-12, therefore a quick and dirty check in configlet could be to match against a regex ~r/[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}/.
This should catch at least obviously invalid UUIDs. If though the package that is used to generate, is also able to parse them, I'd prefer full offline validation, depending on which version we use.
nywilken
commented
6 years ago I think formal validity of an UUID should be checked before even asking upstream for duplicates…
Agreed.
ferhatelmas
commented
6 years ago If though the package that is used to generate, is also able to parse them ...
https://godoc.org/github.com/satori/go.uuid might be a good option.
NobbZ
commented
6 years ago At least the documentation looks good.
I've looked at the currently used UUID generator and its in fact only capable of generating UUIDv4 (randomly generated). And has no option to read or verify UUIDs.
As a matter of fact, I'd suggest not to use plain UUIDv4. I'd generate a single UUID using v1 or v4 by @kytrinyx as a base. We could use this to namespace UUIDv3/v5 per track and then use the tracks UUID as namespace for the exercises.
This way, we will still have low chances of collision, while the UUID per track/exercise combination is deterministic.
I'm not sure though, how we would handle clashes then. With v4 we could simply regenerate, With v3/v5 we could switch version once, and after that do a v4, but to be honest, if we clash 2 times in a row, then we have another kind of trouble :)
tleen
commented
6 years ago Are we seeing UUID clashing as an issue here? Lint will check for a clash although if using the uuid command to generate the chances of having a clash are remote. Most likely it would come in from a copy/paste type action. At which point a new one can be generated. Because @NobbZ is right, if we get two in a row we have bigger problems :smile:
I'm not sure what we use the UUID for in nexterism so understanding that may help. Is it to represent the exercise in the DB or just give it some sort of unique identifier in general?
tleen
commented
6 years ago The uuid generating library is just plain broken it seems (and perhaps abandoned). If we are going by the strict definition of UUID the final group is too long and the library's own tests do not catch it. We should move to a more robust option like the one @ferhatelmas suggested or google/uuid?
kytrinyx
commented
6 years ago The UUID is used to represent the exercise, and it needs to be unique across all exercises on Exercism.
I'd suggest that we use google/uuid.
We have until v2 goes live to get the uuids right in some final/usable form. I would suggest that we switch out the dependent library, and submit PRs to fix all the tracks to fix the UUIDs.
Stargator
commented
6 years ago Is there a summary of what was ultimately the decision?
kytrinyx
commented
6 years ago We decided to:
This doesn't impact the v1 exercism site, but will require a full database refresh of v2 (which was already planned, so we're not losing any data that we thought we would keep around).
kytrinyx
commented
6 years ago Going back to the original title of this issue:
Should we detect invalid UUIDs on CI? (I think it would be worth doing).
tleen
commented
6 years ago @kytrinyx we should during the lint? We can message as to the nature of the failure and how to correct it.
nywilken
commented
6 years ago I agree that checking on CI is crucial, even at lint. But my reservation, and I admit that I have not thought much into the legitimacy of this problem, is that if we run this at lint we will need to change all test data to use valid UUIDs unless there is a disable flag.
Stargator
commented
6 years ago @nywilken Are you saying some tests for exercises may trigger a failure if this is run in lint?
nywilken
commented
6 years ago @Stargator I was specifically speaking about the unit tests for configlet. We currently use UUIDs like “aaa” in configlet fixtures. UUID validation in configlet should not affect tests within exercises.
petertseng
commented
6 years ago Rephrased from question to task.
Decision:
configlet lintshould check whether UUIDs are valid.Support:
Opposition: None.
Task: Make
configlet lintcheck whether UUIDs are valid.Original issue text/question:
What does valid mean? Is it the standard
xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxxwhere each x is a hexadecimal digit?Whatever "valid" means in the Exercism context, would it be good for configlet to check for invalid UUIDs? If whatever is using those UUIDs is expecting them all to be valid, it might be needed.