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(psl, query-engine, schema-engine): add cuid(2) support, fix uuid(7) #5047

Merged
merged 35 commits into from
Nov 22, 2024

Conversation

jkomyno
Copy link
Contributor

@jkomyno jkomyno commented Nov 15, 2024

This PR:

  • Closes Make cuid2 available in @default prisma#17102
  • Adds cuid(2) support
  • Deprecates feat(core): add cuid2 default generator  #4218
  • Deprecates https://github.com/prisma/cuid-rust, as wasm32-unknown-unknown support is already provided by [email protected]. cuid is also now installed as a crate at the workspace level.
  • [Breaking] Serialise @default(uuid(4)) as { "default": { "name": "uuid", "args": [4] }} rather than { "default": { "name": "uuid(4)", "args": [] }} in DMMF
  • [Breaking] Serialise @default(uuid(7)) as { "default": { "name": "uuid", "args": [7] }} rather than { "default": { "name": "uuid(7)", "args": [] }} in DMMF
  • Cleans up DMMF serialisation of ID generators
  • Improves tests after uuid(7) was introduced via Add support for UUIDv7 #4877
  • Fixes re-introspection for uuid (uuid(7) was re-introspected by the Schema Engine as uuid(), which defaults to uuid(4) when using the Query Engine at runtime)
  • Adds new tests for cuid and uuid

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 the driver-adapters (wasm) test suite, specifically when on std::process::id() (e.g., here) and std::time::SystemTime::now() (e.g., here).
I've thus created another fork branch of the latest cuid version in the v1.3.3-wasm32-unknown-unknown branch (see diff).


Some notes for docs writers:

  • @default(cuid()) is interpreted as @default(cuid(1)), so we keep generating cuid v1 by default, as usual.
  • @default(uuid()) is interpreted as @default(uuid(4)), so we keep generating UUID v4 by default, as usual.

Example cuid(1): "cm3rpdbhn0001w10gi9au8cul" (25 chars)
Example cuid(2): "u8yjmh700jcegr5oi3onij1u" (24 chars)

@jkomyno jkomyno added this to the 6.0.0 milestone Nov 15, 2024
@jkomyno jkomyno requested a review from a team as a code owner November 15, 2024 18:46
@jkomyno jkomyno requested review from wmadden and removed request for a team and wmadden November 15, 2024 18:46
Copy link

codspeed-hq bot commented Nov 15, 2024

CodSpeed Performance Report

Merging #5047 will not alter performance

Comparing feat/cuid2-support (bd42901) with main (90c6eb3)

Summary

✅ 11 untouched benchmarks

Copy link
Contributor

github-actions bot commented Nov 15, 2024

WASM Query Engine file Size

Engine This PR Base branch Diff
Postgres 2.044MiB 2.038MiB 6.291KiB
Postgres (gzip) 820.489KiB 818.840KiB 1.649KiB
Mysql 2.010MiB 2.001MiB 8.866KiB
Mysql (gzip) 806.673KiB 804.595KiB 2.079KiB
Sqlite 1.907MiB 1.900MiB 6.850KiB
Sqlite (gzip) 766.352KiB 765.015KiB 1.337KiB

@jkomyno jkomyno changed the title feat(psl): add cuid(2) support feat(psl): add cuid(2) support, fix uuid(7) Nov 15, 2024
@@ -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)),
Copy link
Contributor Author

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.

@@ -16,8 +16,7 @@ pub struct TxId(String);

impl Default for TxId {
fn default() -> Self {
#[allow(deprecated)]
Self(cuid::cuid().unwrap())
Self(cuid::cuid2())
Copy link
Contributor Author

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).

@jkomyno jkomyno requested a review from aqrln November 19, 2024 17:36
@jkomyno jkomyno self-assigned this Nov 19, 2024
Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

amazing work 💯

psl/parser-database/src/attributes/default.rs Show resolved Hide resolved
psl/parser-database/src/attributes/default.rs Outdated Show resolved Hide resolved
psl/parser-database/src/generators.rs Outdated Show resolved Hide resolved
psl/psl/tests/common/asserts.rs Outdated Show resolved Hide resolved
psl/psl/tests/common/asserts.rs Outdated Show resolved Hide resolved
query-engine/query-structure/src/default_value.rs Outdated Show resolved Hide resolved
query-engine/query-structure/src/default_value.rs Outdated Show resolved Hide resolved
@aqrln
Copy link
Member

aqrln commented Nov 20, 2024

@jkomyno heads up: some tests are failing with queries returning different results (could be something sensitive to ordering I guess)

Comment on lines +2 to +11
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
Copy link
Contributor Author

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

#[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);
}
}
, whose dmmf is serialised in the JSON snapshot above (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()
Copy link
Contributor Author

@jkomyno jkomyno Nov 21, 2024

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

jkomyno and others added 7 commits November 21, 2024 01:38
`#!/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.
@jkomyno jkomyno changed the title feat(psl): add cuid(2) support, fix uuid(7) feat(psl, query-engine, schema-engine): add cuid(2) support, fix uuid(7) Nov 21, 2024
Cargo.toml Outdated Show resolved Hide resolved
Comment on lines -51 to -52
# Ignore pnpm-lock.yaml
query-engine/driver-adapters/pnpm-lock.yaml
Copy link
Member

Choose a reason for hiding this comment

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

thanks

Copy link
Member

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?

Copy link
Contributor Author

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"
Copy link
Member

Choose a reason for hiding this comment

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

anything to contribute upstream?

Copy link
Contributor Author

@jkomyno jkomyno Nov 22, 2024

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"
Copy link
Member

@aqrln aqrln Nov 22, 2024

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.

Copy link
Contributor Author

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.

@jkomyno jkomyno merged commit 5e70d19 into main Nov 22, 2024
368 checks passed
@jkomyno jkomyno deleted the feat/cuid2-support branch November 22, 2024 14:24
@mzdr
Copy link

mzdr commented Nov 22, 2024

fantastic work @jkomyno, thank you!

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.

Make cuid2 available in @default
3 participants