-
Notifications
You must be signed in to change notification settings - Fork 241
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(psl, query-engine, schema-engine): add cuid(2)
support, fix uuid(7)
#5047
Conversation
CodSpeed Performance ReportMerging #5047 will not alter performanceComparing Summary
|
WASM Query Engine file Size
|
cuid(2)
supportcuid(2)
support, fix uuid(7)
@@ -310,7 +310,7 @@ fn prisma_value_to_serde(value: &PrismaValue) -> serde_json::Value { | |||
PrismaValue::Float(val) => { | |||
serde_json::Value::Number(serde_json::Number::from_f64(val.to_f64().unwrap()).unwrap()) | |||
} | |||
PrismaValue::Int(val) => serde_json::Value::Number(serde_json::Number::from_f64(*val as f64).unwrap()), | |||
PrismaValue::Int(val) => serde_json::Value::Number(serde_json::Number::from(*val)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was with all likelihood a bug introduced in 34219a9.
…riented fork with cuid 1.3.3
… into feat/cuid2-support
@@ -16,8 +16,7 @@ pub struct TxId(String); | |||
|
|||
impl Default for TxId { | |||
fn default() -> Self { | |||
#[allow(deprecated)] | |||
Self(cuid::cuid().unwrap()) | |||
Self(cuid::cuid2()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed with @aqrln, we're now using cuid2
for interactive transaction IDs (internal API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
amazing work 💯
query-engine/connector-test-kit-rs/query-engine-tests/tests/writes/ids/uuid_create_graphql.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/writes/ids/uuid_create_graphql.rs
Outdated
Show resolved
Hide resolved
query-engine/connector-test-kit-rs/query-engine-tests/tests/writes/ids/uuid_create_graphql.rs
Outdated
Show resolved
Hide resolved
@jkomyno heads up: some tests are failing with queries returning different results (could be something sensitive to ordering I guess) |
id Int @id | ||
createdAt DateTime @default(now()) | ||
someCuid String @default(cuid()) @unique | ||
someCuid1 String @default(cuid(1)) @unique | ||
someCuid2 String @default(cuid(2)) @unique | ||
someUuid String @default(uuid()) @unique | ||
someUuid4 String @default(uuid(4)) @unique | ||
someUuid7 String @default(uuid(7)) @unique | ||
someNanoid String @default(nanoid()) @unique | ||
someNanoidLen6 String @default(nanoid(6)) @unique |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is the Prisma schema file for
prisma-engines/query-engine/dmmf/src/ast_builders/datamodel_ast_builder.rs
Lines 355 to 378 in 04af8ad
#[test] | |
fn test_dmmf_rendering() { | |
let test_cases = fs::read_dir(SAMPLES_FOLDER_PATH) | |
.unwrap() | |
.map(|entry| entry.unwrap().file_name().into_string().unwrap()) | |
.filter(|name| name.ends_with(".prisma")) | |
.map(|name| name.trim_end_matches(".prisma").to_owned()); | |
for test_case in test_cases { | |
println!("TESTING: {test_case}"); | |
let datamodel_string = load_from_file(format!("{test_case}.prisma").as_str()); | |
let dmmf_string = render_to_dmmf(&datamodel_string); | |
if std::env::var("UPDATE_EXPECT") == Ok("1".into()) { | |
save_to_file(&format!("{test_case}.json"), &dmmf_string); | |
} | |
let expected_json = load_from_file(format!("{test_case}.json").as_str()); | |
println!("{dmmf_string}"); | |
assert_eq_json(&dmmf_string, &expected_json, &test_case); | |
} | |
} |
query-engine/dmmf/test_files/functions.json
).
A convenient way to update the dmmf snapshots is:
UPDATE_EXPECT=1 cargo test --package dmmf --features psl/all --lib -- --show-output
pub fn new_cuid() -> Self { | ||
ValueGenerator::new("cuid".to_owned(), vec![]).unwrap() | ||
pub fn new_cuid(version: u8) -> Self { | ||
ValueGenerator::new("cuid".to_owned(), vec![(None, PrismaValue::Int(version as i64))]).unwrap() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first Option<String>
argument of the tuple in Vec<>
seems to be totally useless, btw. This is something we can clean up before Prisma 6
`#!/bin/bash` is non-portable and only works on some systems which has bash in that location, which is not actually standard or necessarily expected. The only two absolute paths one can rely on in portable scripts are `#!/bin/sh` (which is a POSIX standard) and `#!/usr/bin/env` (which is not technically mandated by a formal standard but is de-facto implemented on all Unix systems). None of my systems have a usable bash installation in `/bin/bash`. My Linux system only has bash in `/run/current-system/sw/bin/bash`, so the script fails with `sh: line 2: ./build.sh: cannot execute: required file not found`. My macOS system has an ancient version of bash shipped with the system installed as `/bin/bash` (which Apple last updated in 2007 and will never ever update anymore for license reasons since bash 4+ switched to GPLv3; it's likely to be removed in future macOS versions though as macOS has long replaced bash with zsh as its shell of choice), and in some ways it's worse than not having any since modern bash scripts that assume bash 4 or 5 often fail or produce incorrect results with it. This specific script accidentally happens to work anyway, but the correct bash installation to use on this system is still `/opt/homebrew/bin/bash`. Anything other than `/bin/sh` (be it `bash` or `python` or `node`) can only be dispatched via `/usr/bin/env` if a script is intended to be used on more than one machine because it's not possible to predict where the interpreter is going to be at.
cuid(2)
support, fix uuid(7)
cuid(2)
support, fix uuid(7)
… into feat/cuid2-support
…v1.3.3-wasm32-unknown-unknown"
… into feat/cuid2-support
… race condition in newer versions
# Ignore pnpm-lock.yaml | ||
query-engine/driver-adapters/pnpm-lock.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I don't see it committed in this PR, is also covered by a .gitignore
in some subdirectory?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, let's fix this in a follow-up PR
version = "1.3.2" | ||
source = "git+https://github.com/prisma/cuid-rust?branch=wasm32-support#ccfd958c224c79758c2527a0bca9efcd71790a19" | ||
version = "1.3.3" | ||
source = "git+https://github.com/prisma/cuid-rust?branch=v1.3.3-wasm32-unknown-unknown#dc68c4f47a3dbcd511f605135ac7c948775a3ab9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anything to contribute upstream?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but let's do that after we've shipped Prisma 6
"node": ">=16.13", | ||
"pnpm": ">=8.6.6 <9" | ||
"node": ">=18.18", | ||
"pnpm": ">=8.6.6" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be limited to a specific major version because of different lockfile versions, if we are going to commit the lockfile. If you used pnpm 9 locally then let's just specify pnpm 9.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, let's fix this in a follow-up PR.
fantastic work @jkomyno, thank you! |
This PR:
@default
prisma#17102cuid(2)
supportcuid2
default generator #4218Deprecates https://github.com/prisma/cuid-rust, aswasm32-unknown-unknown
support is already provided by[email protected]
.cuid
is also now installed as a crate at the workspace level.@default(uuid(4))
as{ "default": { "name": "uuid", "args": [4] }}
rather than{ "default": { "name": "uuid(4)", "args": [] }}
in DMMF@default(uuid(7))
as{ "default": { "name": "uuid", "args": [7] }}
rather than{ "default": { "name": "uuid(7)", "args": [] }}
in DMMFuuid(7)
was introduced via Add support for UUIDv7 #4877uuid
(uuid(7)
was re-introspected by the Schema Engine asuuid()
, which defaults touuid(4)
when using the Query Engine at runtime)cuid
anduuid
I originally tried to deprecate https://github.com/prisma/cuid-rust, as
wasm32-unknown-unknown
support was allegedly already provided by[email protected]
. However, that resulted in runtime panics when running thedriver-adapters (wasm)
test suite, specifically when onstd::process::id()
(e.g., here) andstd::time::SystemTime::now()
(e.g., here).I've thus created another fork branch of the latest
cuid
version in thev1.3.3-wasm32-unknown-unknown
branch (see diff).Some notes for docs writers:
@default(cuid())
is interpreted as@default(cuid(1))
, so we keep generatingcuid v1
by default, as usual.@default(uuid())
is interpreted as@default(uuid(4))
, so we keep generatingUUID v4
by default, as usual.Example
cuid(1)
: "cm3rpdbhn0001w10gi9au8cul" (25 chars)Example
cuid(2)
: "u8yjmh700jcegr5oi3onij1u" (24 chars)