-
-
Notifications
You must be signed in to change notification settings - Fork 911
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: parsing non RFC uuid values #455
Changes from 3 commits
214533c
fbc317e
57ab10a
fc6cc23
865e6e7
3fd422b
d6851cb
10e1bfa
b57dc67
f52d4e8
d096cc2
70f4179
2218267
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ | |
}, | ||
"parser": "babel-eslint", | ||
"rules": { | ||
"no-var": ["error"] | ||
"no-var": ["error"], | ||
"curly": ["error", "all"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1345][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})|((00000000-0000-0000-0000-000000000000))$/i; | ||
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
export default uuidRegex; | ||
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,27 @@ | ||
import bytesToUuid from './bytesToUuid.js'; | ||
import validate from './validate.js'; | ||
|
||
// Int32 to 4 bytes https://stackoverflow.com/a/12965194/3684944 | ||
function numberToBytes(num, bytes, offset) { | ||
for (let i = 0; i < 4; ++i) { | ||
const byte = num & 0xff; | ||
// Fill the 4 bytes right-to-left. | ||
bytes[offset + 3 - i] = byte; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this line could warrant a comment. Something like fill the 4 appended bytes right-to-left. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or a link to the respective stackoverflow question 😉 |
||
num = (num - byte) / 256; | ||
} | ||
} | ||
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
function uuidToBytes(uuid) { | ||
// Note: We assume we're being passed a valid uuid string | ||
const bytes = []; | ||
if (!validate(uuid)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simpler, more compact, more performant(???) implementation: // Char offset to hex pairs in uuid strings
const HEX_PAIRS=[0, 2, 4, 6, 9, 11, 14, 16, 19, 21, 24, 26, 28, 30, 32, 34];
function uuidToBytes(uuid) {
if (!validate(uuid)) throw TypeError('Invalid UUID');
return HEX_PAIRS.map(i => parseInt(uuid.slice(i, i + 2), 16));
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @broofa I checked this code performance:
This code is shorter, but less productive. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would again favor conciseness over performance in this case. |
||
return []; | ||
} | ||
|
||
const bytes = new Array(16); | ||
|
||
uuid.replace(/[a-fA-F0-9]{2}/g, function (hex) { | ||
bytes.push(parseInt(hex, 16)); | ||
}); | ||
numberToBytes(parseInt(uuid.slice(0, 8), 16), bytes, 0); | ||
numberToBytes(parseInt(uuid.slice(9, 13) + uuid.slice(14, 18), 16), bytes, 4); | ||
numberToBytes(parseInt(uuid.slice(19, 23) + uuid.slice(24, 28), 16), bytes, 8); | ||
numberToBytes(parseInt(uuid.slice(28), 16), bytes, 12); | ||
|
||
return bytes; | ||
} | ||
|
@@ -28,10 +43,13 @@ export const URL = '6ba7b811-9dad-11d1-80b4-00c04fd430c8'; | |
|
||
export default function (name, version, hashfunc) { | ||
function generateUUID(value, namespace, buf, offset) { | ||
const off = (buf && offset) || 0; | ||
if (typeof value === 'string') { | ||
value = stringToBytes(value); | ||
} | ||
|
||
if (typeof value === 'string') value = stringToBytes(value); | ||
if (typeof namespace === 'string') namespace = uuidToBytes(namespace); | ||
if (typeof namespace === 'string') { | ||
namespace = uuidToBytes(namespace); | ||
} | ||
|
||
if (!Array.isArray(value)) { | ||
throw TypeError('value must be an array of bytes'); | ||
|
@@ -47,12 +65,16 @@ export default function (name, version, hashfunc) { | |
bytes[8] = (bytes[8] & 0x3f) | 0x80; | ||
|
||
if (buf) { | ||
for (let idx = 0; idx < 16; ++idx) { | ||
buf[off + idx] = bytes[idx]; | ||
offset = offset || 0; | ||
|
||
for (let i = 0; i < 16; ++i) { | ||
buf[offset + i] = bytes[i]; | ||
} | ||
|
||
return buf; | ||
} | ||
|
||
return buf || bytesToUuid(bytes); | ||
return bytesToUuid(bytes); | ||
} | ||
|
||
// Function#name is not settable on some platforms (#270) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
import uuidRegex from './regex.js'; | ||
|
||
function validate(uuid) { | ||
return uuidRegex.test(uuid); | ||
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
export default validate; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
import validate from './validate.js'; | ||
|
||
function version(uuid) { | ||
if (validate(uuid)) { | ||
return parseInt(uuid.substr(14, 1), 16); | ||
} | ||
|
||
return -1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here I'm still a bit unsure whether -1 is the best return value for an invalid UUID… @broofa wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ctavan I would recommend always returning the same type from a function…
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
export default version; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import assert from 'assert'; | ||
import version from '../../src/version.js'; | ||
|
||
describe('version', () => { | ||
test('check uuid version', () => { | ||
assert.strictEqual(version('00000000-0000-0000-0000-000000000000'), 0); | ||
|
||
assert.strictEqual(version('d9428888-122b-11e1-b85c-61cd3cbb3210'), 1); | ||
|
||
assert.strictEqual(version('109156be-c4fb-41ea-b1b4-efe1671c5836'), 4); | ||
|
||
assert.strictEqual(version('a981a0c2-68b1-35dc-bcfc-296e52ab01ec'), 3); | ||
|
||
assert.strictEqual(version('90123e1c-7512-523e-bb28-76fab9f2f73d'), 5); | ||
|
||
assert.strictEqual(version('invalid uuid string'), -1); | ||
awwit marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Revert? @ctavan Any opinion here? Since we're using
prettier
, maybe avoid custom rules that are just about esthetics.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re
prettier
: Enforcing curly braces changes the AST and is therefore not in the scope ofprettier
.Re this setting: I was actually hoping that the
"standard"
eslint config would give us a reasonable set of default rules like this one but apparently that's not the case. I personally like https://github.com/airbnb/javascript/tree/master/packages/eslint-config-airbnb but should we decide to include it we should do that in a different PR, that will go too far. It was probably a bad move from my end to propose adding this change to this PR in the first place, apologies! 🙇♂️