test: parsing non RFC uuid values by awwit · Pull Request #455 · uuidjs/uuid

awwit · 2020-05-25T23:55:14Z

Hello!

I added tests for parsing non RFC uuid values. So that next time not to violate the current behavior of the module. (With future v35 optimizations.)

ctavan

Thanks, it's certainly a good thing to cover more corner cases around the way v3/5 UUID namespaces can be passed.

test/unit/v35.test.js

broofa · 2020-05-27T15:02:00Z

The thing about the test cases that you are adding here is that they actually reveal unintended behavior

💯 agree. Codifying this behavior in tests suggests that the ability to accept non-compliant UUIDs is a desirable and intentional trait when, in fact, it's not. The only reason this works is because I was lazy in my initial implementation, for which I apologize.

Proposal:

Fix uuidToBytes so it only accepts valid UUID strings (have it return null otherwise?)
Invert logic of tests added here to validate they don't pass
Add a validate(str) function to public API (just returns uuidToBytes(str) !== null)
Add a version(string) function to public API (returns UUID version number)
Put these changes on a "v9" branch (for future version 9 release) since stricter parse behavior may break existing v3/v5 use cases. But hold off on releasing that branch for a while. These changes aren't urgent enough to push immediately and I suspect our users may be suffering from a bit of major-release fatigue lately.

I believe this would address most of the concerns we've seen around parsing behavior.

BTW, we might want to consider enforcing the valid-ness of UUIDs using a well-crafted regex that we also export publicly. There's significant demand and value in such a regex. While it won't have the performance of @awwit 's previous solution, it will have the benefit of being consistent both internally and externally. For example, it could be used validate form fields such that any values there could be expected to pass whatever validate() function we provide.

Thoughts?

P.S. Note that the somewhat obscure Nil UUID complicates validation logic. I frequently forget that zero is a valid UUID version. 😞

ctavan · 2020-05-27T15:04:25Z

As a side note, deno exposes such validation methods as well: https://github.com/denoland/deno/blob/master/std/uuid/v5.ts#L13

broofa · 2020-05-27T15:08:32Z

As a side note, deno exposes such validation methods as well

Interesting copyright 🤣

ctavan · 2020-05-27T15:10:08Z

Interesting copyright 🤣

Yeah, they only give credit in the main module file

ctavan · 2020-05-27T15:28:52Z

@broofa I agree with all of your points. It would be a nice opportunity to finally attack these longstanding ideas around parsing and probably make this module even more "complete".

Exposing the new APIs in a tree-shakeable way should also minimize impact for existing users.

And I agree that we should move these developments into a new branch and let them mature for a while before we release yet another major version.

awwit · 2020-05-27T18:14:34Z

@ctavan @broofa I made the changes that you asked me for.

But I can’t create v9 branch in your repository to change PR target.

BTW, one more question that I wanted to ask a long time.
Can I change your API to use only Uint8Array (buf argument) in this places?

https://github.com/uuidjs/uuid/blob/eb6038dda7/src/v1.js#L17
https://github.com/uuidjs/uuid/blob/eb6038dda7/src/v4.js#L4
https://github.com/uuidjs/uuid/blob/eb6038dda7/src/v35.js#L30

awwit · 2020-05-27T18:24:00Z

BTW, new way to parse uuid values shows this performance:

uuidv3() x 238,241 ops/sec ±0.88% (92 runs sampled)
uuidv5() x 236,577 ops/sec ±0.71% (95 runs sampled)

vs master:

uuidv3() x 153,787 ops/sec ±0.53% (91 runs sampled)
uuidv5() x 153,972 ops/sec ±0.40% (90 runs sampled)

ctavan

Great work in general! I think we should still put some thoughts into the exact method signatures and return values, but all in all it goes into a very good direction already!

ctavan · 2020-05-27T19:54:56Z

src/regex.js

@@ -0,0 +1,3 @@
+const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})|((00000000-0000-0000-0000-000000000000))$/i;


The [1-5] should really be [1345] since the RFC does not specify v2 UUIDs.

ctavan · 2020-05-27T19:58:19Z

src/v35.js


 function uuidToBytes(uuid) {
-  // Note: We assume we're being passed a valid uuid string
  const bytes = [];


We should also be able to pre-allocate an Array(16) here, shouldn't we?

@ctavan I tested the performance and did not see the difference.
Well, let it be preallocated )

test/unit/v35.test.js

ctavan · 2020-05-27T20:09:14Z

src/v35.js

+
+  for (let i = 0; i < 4; ++i) {
+    const byte = num & 0xff;
+    bytes[offset + 3 - i] = byte;


I think this line could warrant a comment. Something like fill the 4 appended bytes right-to-left.

Or a link to the respective stackoverflow question 😉

ctavan · 2020-05-27T20:10:39Z

src/v35.js

-    bytes.push(parseInt(hex, 16));
-  });
+  if (!validate(uuid)) {
+    return bytes;


If we pre-allocate the bytes = Array(16) then we should return null here.

@ctavan for optimizations from the JS engine, it's best to always return the same type for each function.
Better to return an empty array…

src/v35.js

ctavan · 2020-05-27T20:12:54Z

src/version.js

+    return parseInt(uuid.substr(14, 1), 16);
+  }
+
+  return -1;


Here I'm still a bit unsure whether -1 is the best return value for an invalid UUID… @broofa wdyt?

@ctavan I would recommend always returning the same type from a function…

added comments to numberToBytes func preallocated array in uuidToBytes func

src/regex.js

broofa · 2020-05-27T21:26:12Z

src/regex.js

@@ -0,0 +1,3 @@
+const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})|((00000000-0000-0000-0000-000000000000))$/i;


@ctavan: Version 2 UUIDs are allowed. The RFC doesn't go into detail about them because they're spec'ed in Appendix A (pg. 586) of X/Open DCE: Remote Procedure Call, which is the original spec that heavily inspired/influenced the RFC. FWIW v2 UUIDs are nearly identical to v1, except the node field must be an IEEE [MAC] address, with no allowances for being set randomly. (I'm just now digging into this, btw. I've been happily ignoring this facet of the RFC for a decade. 🥳 )

Suggestions:

Upper case for constant.

Different regex. Use non-capturing group, group has to be around everything between ^ and $ to properly match begin and end of line.

Suggested change

const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})|((00000000-0000-0000-0000-000000000000))$/i;

const REGEX = /^(?:[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}|00000000-0000-0000-0000-000000000000)$/i;

src/validate.js

src/version.js

test/unit/version.test.js

test/unit/v35.test.js

broofa · 2020-05-27T22:57:27Z

src/v35.js

 function uuidToBytes(uuid) {
-  // Note: We assume we're being passed a valid uuid string
-  const bytes = [];
+  if (!validate(uuid)) {


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)); }

@broofa I checked this code performance:

uuidv3() x 198,096 ops/sec ±0.86% (94 runs sampled) uuidv5() x 200,775 ops/sec ±0.55% (95 runs sampled)

This code is shorter, but less productive.

I would again favor conciseness over performance in this case.

broofa · 2020-05-27T23:05:09Z

.eslintrc.json

  "rules": {
-    "no-var": ["error"]
+    "no-var": ["error"],
+    "curly": ["error", "all"]


Why? Revert? @ctavan Any opinion here? Since we're using prettier, maybe avoid custom rules that are just about esthetics.

re prettier: Enforcing curly braces changes the AST and is therefore not in the scope of prettier.

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! 🙇‍♂️

src/v35.js

test/unit/v35.test.js

Co-authored-by: Robert Kieffer <robert@broofa.com>

src/regex.js

ctavan · 2020-05-28T18:44:39Z

.eslintrc.json

  "rules": {
-    "no-var": ["error"]
+    "no-var": ["error"],
+    "curly": ["error", "all"]


re prettier: Enforcing curly braces changes the AST and is therefore not in the scope of prettier.

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! 🙇‍♂️

src/index.js

src/regex.js

ctavan · 2020-05-28T18:55:04Z

src/v35.js

 function uuidToBytes(uuid) {
-  // Note: We assume we're being passed a valid uuid string
-  const bytes = [];
+  if (!validate(uuid)) {


I would again favor conciseness over performance in this case.

test/unit/validate.test.js

…lues

awwit · 2020-05-29T16:11:00Z

@ctavan @broofa I edited the babel configuration a bit. Specified the version of IE for which the module will be built.

https://babeljs.io/docs/en/babel-preset-env

esmBrowser: {
  presets: [['@babel/preset-env', { targets: { ie: '11' }, modules: false }]],
},

Or is it superfluous?

ctavan · 2020-05-29T19:02:55Z

.babelrc.js

    },
    esmBrowser: {
-      presets: [['@babel/preset-env', { modules: false }]],
+      presets: [['@babel/preset-env', { targets: { ie: '11' }, modules: false }]],


I just checked again and I think this is not necessary:

Sidenote, if no targets are specified, @babel/preset-env will transform all ECMAScript 2015+ code by default.

@ctavan is explicit better than implicit? =)

ctavan

Thank you so much for your effort and especially for your patience with me 😄

I'll merge this later tonight

This reverts commit d096cc2.

broofa

Approving. I've got some thoughts on uuidToBytes(), but I'll put those up in separate PR once this is on a v9 branch.

Also, echoing what @ctavan said about your effort and patience. Thank you!

test: parsing non RFC uuid values

214533c

ctavan requested changes May 26, 2020

View reviewed changes

test/unit/v35.test.js Outdated Show resolved Hide resolved

awwit requested a review from ctavan May 27, 2020 10:53

ctavan requested changes May 27, 2020

View reviewed changes

test/unit/v35.test.js Outdated Show resolved Hide resolved

feat: create validate, version, uuidRegex (and import to public API)

fbc317e

awwit requested a review from ctavan May 27, 2020 18:15

ctavan requested changes May 27, 2020

View reviewed changes

style: always curly braces

57ab10a

added comments to numberToBytes func preallocated array in uuidToBytes func

broofa requested changes May 27, 2020

View reviewed changes

awwit and others added 4 commits May 28, 2020 02:42

Update src/regex.js

fc6cc23

Co-authored-by: Robert Kieffer <robert@broofa.com>

Update src/validate.js

865e6e7

Co-authored-by: Robert Kieffer <robert@broofa.com>

Update src/version.js

3fd422b

Co-authored-by: Robert Kieffer <robert@broofa.com>

Update test/unit/version.test.js

d6851cb

Co-authored-by: Robert Kieffer <robert@broofa.com>

broofa reviewed May 28, 2020

View reviewed changes

src/regex.js Outdated Show resolved Hide resolved

awwit added 2 commits May 28, 2020 03:20

feat: throw error when invalid uuid

10e1bfa

test: validate function

b57dc67

ctavan requested changes May 28, 2020

View reviewed changes

awwit added 2 commits May 29, 2020 16:25

Merge remote-tracking branch 'fork/master' into test/parse-non-rfc-va…

f52d4e8

…lues

feat: short version of parsing UUID

d096cc2

awwit requested review from broofa and ctavan May 29, 2020 16:19

ctavan mentioned this pull request May 29, 2020

chore: improve code style #460

Merged

ctavan reviewed May 29, 2020

View reviewed changes

fix: remove explicit ie babel target

70f4179

ctavan approved these changes May 29, 2020

View reviewed changes

Revert "feat: short version of parsing UUID"

2218267

This reverts commit d096cc2.

ctavan force-pushed the test/parse-non-rfc-values branch from f30b4a7 to 2218267 Compare May 30, 2020 12:33

broofa approved these changes May 30, 2020

View reviewed changes

ctavan mentioned this pull request Jun 2, 2020

Expose bytes to uuid #180

Closed

broofa changed the base branch from master to v9 June 2, 2020 15:34

broofa merged commit ddb7ee9 into uuidjs:v9 Jun 2, 2020

broofa mentioned this pull request Jun 2, 2020

fix: concise uuidToBytes #463

Closed

broofa mentioned this pull request Jul 28, 2020

Provide regular expressions and JSON schemas for UUIDs #498

Closed

		@@ -0,0 +1,3 @@
		const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})\|((00000000-0000-0000-0000-000000000000))$/i;

	const uuidRegex = /^([0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12})\|((00000000-0000-0000-0000-000000000000))$/i;
	const REGEX = /^(?:[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}\|00000000-0000-0000-0000-000000000000)$/i;

Uh oh!

Conversation

awwit commented May 25, 2020

Uh oh!

ctavan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

broofa commented May 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctavan commented May 27, 2020

Uh oh!

broofa commented May 27, 2020

Uh oh!

ctavan commented May 27, 2020

Uh oh!

ctavan commented May 27, 2020

Uh oh!

awwit commented May 27, 2020

Uh oh!

awwit commented May 27, 2020

Uh oh!

ctavan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

broofa commented May 27, 2020 •

edited

Loading

awwit commented May 29, 2020 •

edited

Loading

awwit May 29, 2020 •

edited

Loading

broofa left a comment •

edited

Loading