Use crypo RNG for auto ID generation to reduce conflicts. #2764
Conversation
| @@ -2442,7 +2442,7 @@ export class CollectionReference<T = firestore.DocumentData> extends Query<T> | |||
| // We allow omission of 'pathString' but explicitly prohibit passing in both | |||
| // 'undefined' and 'null'. | |||
| if (arguments.length === 0) { | |||
| pathString = AutoId.newId(); | |||
| pathString = AutoId.newId(PlatformSupport.getPlatform()); | |||
There was a problem hiding this comment.
AutoId.newId should just call PlatformSupport.getPlatform() for itself. If you need a version of this that takes the platform as an argument for testing, it's fine to have AutoId.newId() delegate to that version. We shouldn't have to update all callers to fix this.
| * Generates a random byte `b` with `0 <= b <= max`. If `max` is negative, | ||
| * it is the same as setting it to byte max (255). | ||
| */ | ||
| randomByte(max: number): number; |
There was a problem hiding this comment.
The platforms both provide ways to get an array of random bytes. It's better to keep this interface structured in terms of exposing common platform primitives and than we can implement any derived concerns (like limiting to a max value just once).
Concretely, I think this should be randomBytes that takes a number of bytes to generate and returns a Uint8Array of those contents. (Note that Node's crypto.randomBytes returns a Buffer but that's a subclass of Uint8Array so it's a suitable return value).
packages/firestore/src/util/misc.ts
Outdated
| // Alphanumeric characters | ||
| const chars = | ||
| 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; | ||
| let autoId = ''; | ||
| for (let i = 0; i < 20; i++) { | ||
| autoId += chars.charAt(Math.floor(Math.random() * chars.length)); | ||
| // Length of `chars` is 62. We generate random values between 0 and 247. |
There was a problem hiding this comment.
Mark this explicitly inclusive. Usually "between" implies one or both end points are excluded from the set. That is, use "Random values between 0 and 247 (inclusive)."
packages/firestore/src/util/misc.ts
Outdated
| // Each value maps to an 4-element interval of `chars`, for example: | ||
| // 0..3 -> 0, 4..7 -> 1, etc. This ensures a uniform distribution of | ||
| // probability for all candidates. | ||
| const v = platform.randomByte(247); |
There was a problem hiding this comment.
A possible improvement here is to request 20-40 random bytes at a time from platform.getBytes() to save the overhead of individual calls. At the very least this would save the overhead of allocating an array for every call.
packages/firestore/src/util/misc.ts
Outdated
| // Alphanumeric characters | ||
| const chars = | ||
| 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'; | ||
| let autoId = ''; | ||
| for (let i = 0; i < 20; i++) { | ||
| autoId += chars.charAt(Math.floor(Math.random() * chars.length)); | ||
| // Length of `chars` is 62. We generate random values between 0 and 247. |
| * Generates a random byte `b` with `0 <= b <= max`. If `max` is negative, | ||
| * it is the same as setting it to byte max (255). | ||
| */ | ||
| randomByte(max: number): number; |
packages/firestore/src/util/misc.ts
Outdated
| // Each value maps to an 4-element interval of `chars`, for example: | ||
| // 0..3 -> 0, 4..7 -> 1, etc. This ensures a uniform distribution of | ||
| // probability for all candidates. | ||
| const v = platform.randomByte(247); |
| @@ -2442,7 +2442,7 @@ export class CollectionReference<T = firestore.DocumentData> extends Query<T> | |||
| // We allow omission of 'pathString' but explicitly prohibit passing in both | |||
| // 'undefined' and 'null'. | |||
| if (arguments.length === 0) { | |||
| pathString = AutoId.newId(); | |||
| pathString = AutoId.newId(PlatformSupport.getPlatform()); | |||
| autoId += chars.charAt(Math.floor(Math.random() * chars.length)); | ||
| while (autoId.length < 20) { | ||
| const bytes = PlatformSupport.getPlatform().randomBytes(40); | ||
| bytes.forEach(b => { |
There was a problem hiding this comment.
You could rephrase this with a single for loop:
const numBytes = 40;
const maxValue = 62 * 4;
let bytes = PlatformSupport.getPlatform().randomBytes(numBytes);
let i = 0;
while (autoId.length < 20) {
if (i >= numBytes) {
bytes = PlatformSupport.getPlatform().randomBytes(numBytes);
i = 0;
}
const b = bytes[i++];
// Avoid modulus bias by discarding byte values greater than 247.
if (b < maxValue) {
autoId += chars.charAt(b % 62)
}
}Not sure this is necessarily better.
There was a problem hiding this comment.
Right..it's hard to say which one is better. I'll stick to my version..
packages/firestore/src/util/misc.ts
Outdated
| // Length of `chars` is 62. We only take bytes between 0 and 62*4-1 | ||
| // (both inclusive). The value is then evenly mapped to indices of `char` | ||
| // via a modulo operation. | ||
| if (autoId.length < 20 && b <= 62 * 4 - 1) { |
There was a problem hiding this comment.
This magic number deserves a constant declaration.
)" This reverts commit a85f81d.
No description provided.