paralleldrive / cuid

Collision-resistant ids optimized for horizontal scaling and performance.
Other
3.42k stars 123 forks source link

It seems scientific notation random values are still possible #200

Closed fireflysemantics closed 1 year ago

fireflysemantics commented 4 years ago

I created this test:

import { getRandomValue } from './getRandomValue'

const containsDashes = x => (/-/).test(x);

it(`Should return only positive non dashed random values`, () => {
    expect(Array.from({length: 1000000}, getRandomValue).
    filter(containsDashes).length).toEqual(0)
});

For this conversion:

import * as crypto from "crypto"

var lim = Math.pow(2, 32) - 1;

export function getRandomValue () {
  return Math.abs(crypto.randomBytes(4)
    .readInt32BE(0) / lim)
}

It does return a dashed random value:

  ● Should return only positive non dashed random values

    expect(received).toEqual(expected) // deep equality

    Expected: 0
    Received: 1

      5 | it(`Should return only positive non dashed random values`, () => {
      6 |     expect(Array.from({length: 1000000}, getRandomValue).
    > 7 |     filter(containsDashes).length).toEqual(0)
        |                                    ^
      8 | });

      at Object.<anonymous> (projects/cuid/src/lib/lib/getRandomValue.spec.ts:7:36)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 total
Snapshots:   0 total
Time:        5.978s
Ran all test suites.
npm ERR! Test failed.  See above for more details.
ole@mkt:~/Github/cuid$ 
fireflysemantics commented 4 years ago

The test is also "Random" in nature. If I run it 3 times in a row, 1 run will pass. The other two will have 1 or 2 random numbers with dashes in them.

fireflysemantics commented 4 years ago

It seems one possible solution is to use % instead of /:

https://stackoverflow.com/questions/60676476/eliminating-dashes-from-node-crypto-generated-random-values

I ran 10 million samples and non contain dashes.

fireflysemantics commented 4 years ago

Created an Angular Package Format version of cuid here:

https://www.npmjs.com/package/@fireflysemantics/cuid

This is primarily for the browser / typescript use case.

https://medium.com/@ole.ersoy/generating-collision-resistant-fast-unique-ids-d7a9d1286d9c

ericelliott commented 4 years ago

Please open a PR

fireflysemantics commented 4 years ago

Just change:

export function getRandomValue () {
  return Math.abs(crypto.randomBytes(4)
    .readInt32BE(0) / lim)
}

to:

export function getRandomValue () {
  return Math.abs(crypto.randomBytes(4)
    .readInt32BE(0) % lim)
}
ericelliott commented 1 year ago

Please reopen if this issue still exists.