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

feat: replace uuid with prefixed string with schemaIdCounter #661

Merged
merged 1 commit into from
Nov 12, 2023

Conversation

Uzlopak
Copy link
Contributor

@Uzlopak Uzlopak commented Nov 6, 2023

We dont need crypto randomUUID, because we are cool. We use an internal counter and a fsj specific prefic to generate the schema.id.

@Ethan-Arrowood
@inlet

Checklist

let alwaysLength36 = true
let validUUID4 = true
for (let i = 0; i < 1e6; ++i) {
const uuid = uuidV4()

Check failure

Code scanning / CodeQL

Insecure randomness High test

This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
This uses a cryptographically insecure random number generated at
Math.random()
in a security context.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, we dont need cryptographical secure random number generation. We are just using the uuid for internal ids of the merged schemas etc..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be extremely wary of this. Maybe schema ids are not super secretive, but the are meant to be globally unique and my uninformed guess is that Math.random is too predictable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For our use case it should be good enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know why github bot resolved this conversiation. I think @jsumners thought is important but still I think it is neglectable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to try out to solve this silly code:
performance.now() works on node and browses

test/uuid-v4.test.js Fixed Show fixed Hide fixed
Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, crypto.randomUUID is using pre-allocated cache.
Is it common to have that large (128 times) of id generate which exceed the cache entropy?

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 7, 2023

I guess it is for faster uuid generation. Like randomInt.

@mcollina
Copy link
Member

mcollina commented Nov 7, 2023

If you need a unique identifier for just one process, you can use a global progressive integer.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 12, 2023

I personally think that collision wise it is easier to use this uuid implementation, because an incrementing number could result in collisions if multiple compiled json stringify are concentrated in one function.

I think this is the cheapest solution, while also keeping it somewhat unique. I dont think that collisions are possible.

@mcollina
Copy link
Member

because an incrementing number could result in collisions if multiple compiled json stringify are concentrated in one function.

How? We don't support combining functions.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 12, 2023

I am worried that using just an incrementing number could result in some unexpected behavior in some dependent projects. Do we call then the internal schemas __fjs_${schemaId}

@ivan-tymoshenko
Copy link
Member

Adding an incremental counter + internal prefix name should work.

@Uzlopak
Copy link
Contributor Author

Uzlopak commented Nov 12, 2023

@mcollina
@ivan-tymoshenko

Implemented accordingly.

@Uzlopak Uzlopak changed the title feat: integrate uuid-v4 generator feat: replace uuid with prefixed string with schemaIdCounter Nov 12, 2023
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit 4f6f7b8 into master Nov 12, 2023
21 checks passed
@mcollina mcollina deleted the uuid-v4 branch November 12, 2023 23:21
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

Successfully merging this pull request may close these issues.

6 participants