Skip to content
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

Length of short.generate() is not consistent #39

Closed
deepinder19 opened this issue Apr 23, 2020 · 11 comments
Closed

Length of short.generate() is not consistent #39

deepinder19 opened this issue Apr 23, 2020 · 11 comments

Comments

@deepinder19
Copy link

short.generate() usually returns uuid of 22 characters but sometimes it just returns 21 characters. There is no straightforward way to reproduce this but it happens sometimes if I keep on calling the function.

Is this some known behaviour or an issue that needs to be addressed?

@jeremyblalock
Copy link

A UUID is just a 128-bit number, under the hood. So my guess is it's not zero-padding, and so it's just being represented one character (or more) shorter if it's smaller.

@papb
Copy link

papb commented Apr 27, 2020

@jeremyblalock Should we call padStart on the result of short-uuid to make it always return 22 characters consistently? Can we pad with zeroes? Or this way it will no longer be unique?

@jeremyblalock
Copy link

jeremyblalock commented Apr 27, 2020

From what I can tell, calling padStart (or some other type of leftpad) should be completely fine. Internally short-uuid is using any-base (https://github.com/HarasimowiczKamil/any-base), which doesn't provide any padding options. short-uuid used Flickr's implementation of base-58 which uses 1 as the zero bit (because base-58 doesn't use zero or capital o), so padding with 1's should be totally fine.

Here's info on the Flickr base-58 alphabet: https://en.wikipedia.org/wiki/Base58

@oculus42
Copy link
Owner

The variable length is a known behavior. For some values and alphabets it will vary by more than one character.

We could add a padStart or equivalent option. It would need to calculate the max length of an Id in that alphabet to pad consistently., As @jeremyblalock suggests, it should use the alphabet to pad so translating the value back to a UUID works as expected, even with older (current) versions of short-uuid.

@papb
Copy link

papb commented Apr 28, 2020

@jeremyblalock Thanks for the explanation! Just to make sure I understand, this means that the current implementation never outputs a short-uuid starting with the character 1, right?

(assuming the default alphabet, which is Flickr Base58)

@jeremyblalock
Copy link

@oculus42 maybe you can confirm, but I believe that's the case.

@oculus42
Copy link
Owner

oculus42 commented May 2, 2020

I can confirm. This should work for any alphabet by using first character of the translator alphabet translator.alphabet[0]

You can validate this for yourself with the same code I used:

const validatePadStart = (translator, entries = 100) => {
    const zeroValue = translator.alphabet[0];
    const data = Array(entries).fill('').map(translator.generate);
    const maxLen = data.reduce((result, val) => val.length > result ? val.length : result, 0);
    const allMatch =  data.every(val => translator.toUUID(val) === translator.toUUID(val.padStart(maxLen, zeroValue)));
    return allMatch;
}

@papb
Copy link

papb commented May 2, 2020

Awesome. So maybe an option could be added to generate such as:

translator.generate({ consistentLength: true })

which would always return a string with the same length?

function generate(options) {
  if (options.consistentLength) {
    const length = figureOutWhatIsTheMaxLengthPossiblyGeneratedInAlphabet(alphabet);
    return generate().padStart(length, alphabet[0]);
  }
  // ...
}

@thadeucity
Copy link
Contributor

thadeucity commented Sep 5, 2020

Hi, i was having the same "issue", thought about it and sent a pull request with a simple feature to "solve" this.

Since the UUID is 128 bit long, rounding up the division between the Logarithm of ( 2 ^ 128 ) and ( alphabet length ) will give you the maximum length the Short Id will ever have.

With that and the first Char of the alphabet I changed the function to pad the result accordingly.

If my pull request have any inconsistency please let me know.

@thadeucity
Copy link
Contributor

Inserted a better solution with options as suggested by @papb

@oculus42
Copy link
Owner

oculus42 commented Sep 7, 2020

With thanks to @thadeucity, v4.0.1 is out with consistent-length shortened values by default.

@oculus42 oculus42 closed this as completed Sep 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants