prisma-engines icon indicating copy to clipboard operation
prisma-engines copied to clipboard

Add support for UUIDv7

Open mcuelenaere opened this issue 1 year ago • 7 comments

This implements https://github.com/prisma/prisma/issues/24079. The change is pretty minimal, widening the support from @default(uuid()) to @default(uuid(7)).

It leverages the existing uuid crate instead of adding a new one (uuid7).

~Right now this code is mostly untested (apart from the tests I added to psl), because I couldn't get the tests running locally. I'd appreciate some guidance in this area.~ CI runs fine

mcuelenaere avatar May 20 '24 12:05 mcuelenaere

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 20 '24 12:05 CLAassistant

uuid seems to be behind uuid7 with regards to how accurately it represents the spec. See discussion here: https://github.com/uuid-rs/uuid/issues/717

kibertoad avatar May 20 '24 12:05 kibertoad

CodSpeed Performance Report

Merging #4877 will not alter performance

Comparing mcuelenaere:feature/uuidv7 (81642e5) with main (34ace0e)

Summary

✅ 11 untouched benchmarks

codspeed-hq[bot] avatar May 21 '24 08:05 codspeed-hq[bot]

FYI: after this gets an approval, I'll rebase + squash the fixup commits

mcuelenaere avatar Jun 12 '24 15:06 mcuelenaere

@Weakky Gentle ping. Is anything missing in this PR?

kibertoad avatar Jun 18 '24 08:06 kibertoad

@mcuelenaere Thanks for the quick review fix. I have run the tests. Would you mind having a look at the failures?

Weakky avatar Jun 24 '24 16:06 Weakky

@Weakky I am able to run the tests for sqlite locally, but they seem to fail in CI for the driver adapters. However, I can't seem to easily run these locally + the output of the CI does not give any indication as to what's going wrong (just that it's failing).

Any help would be appreciated.

mcuelenaere avatar Jun 25 '24 16:06 mcuelenaere

follow

xiaolongkipsi avatar Jul 08 '24 18:07 xiaolongkipsi

I've rebased on current main, as that seems stable and the tests were failing due to something else the last time

mcuelenaere avatar Jul 21 '24 13:07 mcuelenaere

I've also updated uuid to v1.10.0 to address @kibertoad 's comment

mcuelenaere avatar Jul 21 '24 13:07 mcuelenaere

@Weakky Is there anything missing?

kibertoad avatar Jul 29 '24 10:07 kibertoad

@mcuelenaere I have investigated why tests were failing. Turns out SystemTime::now is not implemented yet for Wasm. https://github.com/rust-lang/rust/issues/48564

Fortunately, uuid has a "js" feature which works around this limitation.

https://github.com/uuid-rs/uuid/blob/main/src/timestamp.rs#L302-L327

Could you please add the "js" feature for the uuid crate?

Thanks

Weakky avatar Jul 31 '24 12:07 Weakky

Aha, good catch! I've pushed a fixup commit for that.

I assume just straight-up adding the feature to uuid is fine and it does not need to be in some kind of [target.'cfg(target_arch = "wasm32")'.dependencies] context, right?

mcuelenaere avatar Jul 31 '24 18:07 mcuelenaere