UUID convenience class

[steffenagger]

UUID convenience class

Convenience class making uuid creations easier & typed, allowing:

const uuid1 = new UUID();
const uuid2 = new UUID('aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaaaa');
const uuid3 = new UUID('aaaaaaaaaaaa4aaaaaaaaaaaaaaaaaaa');

const bin = uuid1.toBinary();
const uuid11 = bin.toUUID(); // will throw if not of sub_type Binary.SUBTYPE_UUID

I've kept this PR as a draft, as there are still left in comments as // NOTE: that should be addressed.

Changes/approaches:

• Introduced a UUID convenience class.
  • Helper-methods on instances of UUID and Binary to convert between eachother.
• The implementation is kept free of additional dependencies, despite the uuid looking as a solid candidate (uuid package is still added as a dev-dependency for string validation in tests).
  • uuid's helper functions work only with strings. For validation converting the underlying buffer into a string would have been necessary - this seemed wrong in relation to perfomance - it also dictates a certain string format (must include dashes e.g.: 'xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx').
• Expanded crypto detection to include global, for allowing packages such as react-native-get-random-values to populate implement global.crypto. getRandomValues in the context of React Native.
  • A warning is triggered when randomBytes detection falls back to insecureRandomBytes (with added guidance for React Native).
• [value] instanceof Buffer has been replaced with Buffer.isBuffer(value), as instanceof yields problems when multiple packages depends on Buffer implementations, in browser builds. (as each package is scoped)

[kneth]

Is this a good place to discuss how random data is generated?

[nbbeeken]

Can we rely on the 'uuid' dependency here? will it correctly find the the right crypto function across platforms? If not I believe it allows you to provide the correct function as an argument.

[steffenagger]

I've added a warning if/when the current implementation falls back to using insecureRandomBytes.

[nbbeeken]

There's some changes needed here to bring us in line with spec and I don't see why we can't use the uuid library as a dependency, hopefully that'll make life a bit easier here. Thanks for taking a crack at this!

[nbbeeken]

Can we rely on the 'uuid' dependency here? will it correctly find the the right crypto function across platforms? If not I believe it allows you to provide the correct function as an argument.

[steffenagger]

This is a nondeterministic test (as it tests auto-generation). Is this welcome as a test case?

[emadum]

One minor nit, but this LGTM, thanks for the contribution! 👍

[emadum]

I think it'd be better to use process.emitWarning here.

Suggested change

	console.warn(insecureWarning);
	process.emitWarning(insecureWarning);

[steffenagger]

Thank you, but wouldn't this throw in a browser context without crypto?

[nbbeeken]

LGTM 👍

[jamiter]

Thanks for all the work done to get this in!

A question though: The UUID BSON type isn't handled in the serializer:

js-bson/src/parser/serializer.ts

Lines 774 to 846 in 210fc11

	if (typeof value === 'string') {
	index = serializeString(buffer, key, value, index, true);
	} else if (typeof value === 'number') {
	index = serializeNumber(buffer, key, value, index, true);
	} else if (typeof value === 'bigint') {
	throw new BSONTypeError('Unsupported type BigInt, please use Decimal128');
	} else if (typeof value === 'boolean') {
	index = serializeBoolean(buffer, key, value, index, true);
	} else if (value instanceof Date || isDate(value)) {
	index = serializeDate(buffer, key, value, index, true);
	} else if (value === undefined) {
	index = serializeNull(buffer, key, value, index, true);
	} else if (value === null) {
	index = serializeNull(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'ObjectId' || value['_bsontype'] === 'ObjectID') {
	index = serializeObjectId(buffer, key, value, index, true);
	} else if (isUint8Array(value)) {
	index = serializeBuffer(buffer, key, value, index, true);
	} else if (value instanceof RegExp || isRegExp(value)) {
	index = serializeRegExp(buffer, key, value, index, true);
	} else if (typeof value === 'object' && value['_bsontype'] == null) {
	index = serializeObject(
	buffer,
	key,
	value,
	index,
	checkKeys,
	depth,
	serializeFunctions,
	ignoreUndefined,
	true,
	path
	);
	} else if (
	typeof value === 'object' &&
	isBSONType(value) &&
	value._bsontype === 'Decimal128'
	) {
	index = serializeDecimal128(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'Long' || value['_bsontype'] === 'Timestamp') {
	index = serializeLong(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'Double') {
	index = serializeDouble(buffer, key, value, index, true);
	} else if (typeof value === 'function' && serializeFunctions) {
	index = serializeFunction(buffer, key, value, index, checkKeys, depth, true);
	} else if (value['_bsontype'] === 'Code') {
	index = serializeCode(
	buffer,
	key,
	value,
	index,
	checkKeys,
	depth,
	serializeFunctions,
	ignoreUndefined,
	true
	);
	} else if (value['_bsontype'] === 'Binary') {
	index = serializeBinary(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'Symbol') {
	index = serializeSymbol(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'DBRef') {
	index = serializeDBRef(buffer, key, value, index, depth, serializeFunctions, true);
	} else if (value['_bsontype'] === 'BSONRegExp') {
	index = serializeBSONRegExp(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'Int32') {
	index = serializeInt32(buffer, key, value, index, true);
	} else if (value['_bsontype'] === 'MinKey' || value['_bsontype'] === 'MaxKey') {
	index = serializeMinMax(buffer, key, value, index, true);
	} else if (typeof value['_bsontype'] !== 'undefined') {
	throw new BSONTypeError('Unrecognized or invalid _bsontype: ' + value['_bsontype']);
	}
	}

I was wondering if this is on purpose, or an oversight. And would a PR be accepted to add this type to the list?

[jamiter]

For anyone having the same issue. This seems to solve it for me:

Object.defineProperty(UUID.prototype, 'toBSON', {
    get() {
        return this.toBinary;
    },
});

The toBSON is called before serializing the value and it's perfectly able to save the binary.

Or was to intended behaviour to call it manually for each value on each save? That get's problematic quickly, especially with nested UUIDs.

Edit: There is still the issue of deserializing though.