Skip to content

fix: prevent breaking default expr in SQLite #5748

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.

Sign up for GitHub

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

Open
ken0x0a wants to merge 5 commits into
base: main
Choose a base branch
from
+27 −9
Open

fix: prevent breaking default expr in SQLite #5748

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3088ecd
fix: prevent breaking default expr
ken0x0a Jan 22, 2026
5d7398d
fix: handle multi byte char using `starts_with` & `ends_with`
ken0x0a Feb 10, 2026
a4dbe4e
fix: wrap all column types
ken0x0a Feb 10, 2026
447aaff
fix: condition
ken0x0a Feb 25, 2026
ee8ace3
Merge branch 'main' into fix/sql-default-expr
jacek-prisma Feb 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions schema-engine/sql-introspection-tests/tests/re_introspection/sqlite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ async fn multiple_changed_relation_names_due_to_mapped_models(api: &mut TestApi)
t.add_column("id", types::primary());
t.add_column("user_id", types::integer().nullable(false).unique(true));
t.add_column("user_id2", types::integer().nullable(false).unique(true));
t.add_column(
"createdAt",
types::custom("INTEGER NOT NULL DEFAULT (CAST(unixepoch('subsec') * 1000 AS INTEGER))"),
);
Comment on lines +16 to +19
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Consider adding a dedicated test for the default expression fix instead of modifying an unrelated test.

This test is named multiple_changed_relation_names_due_to_mapped_models and focuses on relation renaming. Adding createdAt with a complex default expression here conflates two unrelated concerns. A standalone test specifically for the SQLite default expression parenthesization fix would:

  1. Make the regression coverage self-documenting and easy to find.
  2. Avoid coupling — if the relation-renaming test is ever removed or refactored, the default-expression coverage silently disappears.

Also, please ensure the expect! snapshot was regenerated via UPDATE_EXPECT=1 cargo test. As per coding guidelines, "When updating PSL diagnostics or error messages, always regenerate expect! snapshot tests using UPDATE_EXPECT=1 cargo test to ensure test assertions match new output."

Also applies to: 36-36, 55-55

Copy link
Contributor

@jacek-prisma jacek-prisma Mar 3, 2026

Choose a reason for hiding this comment

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

This comment is valid, it might make sense to introduce a similar test that just covers this

Copy link

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

@jacek-prisma, thank you for confirming! I'd be happy to help create a dedicated test that specifically covers the SQLite default expression parenthesization fix. This will make the regression coverage clear and independent of the relation-renaming test.

Would you like me to create a new test function that:

  1. Sets up a table with a column using a complex default expression (like the unixepoch example)
  2. Performs introspection to verify the default is correctly wrapped in parentheses
  3. Tests re-introspection to ensure the default expression remains stable

Task: Add dedicated test for SQLite default expression parenthesization

  • Create new test function named sqlite_default_expression_with_parentheses in the same file
  • Set up a simple table (e.g., "Post") with a column using a complex default expression
  • Verify initial introspection wraps the default in parentheses correctly
  • Verify re-introspection preserves the parenthesized default expression
  • Keep the test focused solely on default expression handling (no relations, no other concerns)

💡 Use @coderabbitai plan to create a plan for code changes


t.add_foreign_key(&["user_id"], "User", &["id"]);
t.add_foreign_key(&["user_id2"], "User", &["id"]);
Expand All @@ -29,6 +33,7 @@ async fn multiple_changed_relation_names_due_to_mapped_models(api: &mut TestApi)
id Int @id @default(autoincrement())
user_id Int @unique
user_id2 Int @unique
createdAt Int @default(dbgenerated("(CAST(unixepoch('subsec') * 1000 AS INTEGER))"))
custom_User Custom_User @relation("CustomRelationName", fields: [user_id], references: [id], onDelete: NoAction, onUpdate: NoAction)
custom_User2 Custom_User @relation("AnotherCustomRelationName", fields: [user_id2], references: [id], onDelete: NoAction, onUpdate: NoAction)
}
Expand All @@ -47,6 +52,7 @@ async fn multiple_changed_relation_names_due_to_mapped_models(api: &mut TestApi)
id Int @id @default(autoincrement())
user_id Int @unique(map: "sqlite_autoindex_Post_1")
user_id2 Int @unique(map: "sqlite_autoindex_Post_2")
createdAt Int @default(dbgenerated("(CAST(unixepoch('subsec') * 1000 AS INTEGER))"))
custom_User2 Custom_User @relation("AnotherCustomRelationName", fields: [user_id2], references: [id], onDelete: NoAction, onUpdate: NoAction)
custom_User Custom_User @relation("CustomRelationName", fields: [user_id], references: [id], onDelete: NoAction, onUpdate: NoAction)
}
Expand Down
30 changes: 21 additions & 9 deletions schema-engine/sql-schema-describer/src/sqlite.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,18 @@ impl<'a> SqlSchemaDescriber<'a> {
}
}

/// Wraps the given string in parentheses if it is not already.
/// The result of `PRAGMA table_info` removes parentheses from default values,
/// so we need to add parentheses when generating prisma schema.
/// See <https://github.com/prisma/prisma/issues/29064>
fn wrap_in_parentheses(s: &str) -> Cow<'_, str> {
if !s.starts_with('(') && s.ends_with(')') {
Copy link
Contributor

@jacek-prisma jacek-prisma Feb 10, 2026

Choose a reason for hiding this comment

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

I think this is meant to be
!(s.starts_with('(') && s.ends_with(')'))

Copy link
Author

@ken0x0a ken0x0a Feb 10, 2026

Choose a reason for hiding this comment

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

No, it's not.

We need to wrap with parentheses as PRAGMA table_info removes parentheses from default values.

Copy link
Contributor

@jacek-prisma jacek-prisma Feb 10, 2026

Choose a reason for hiding this comment

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

I think your condition is wrong though, it checks whether the expxression DOES NOT start with ( and DOES end with ). I think the intended condition is whether it DOES NOT start with ( and DOES NOT end with ).

Copy link
Author

@ken0x0a ken0x0a Feb 11, 2026

Choose a reason for hiding this comment

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

Oops,
I was wrong.
Sorry.

But I'm worried about some special case like CURRENT_TIMESTAMP.
If there is other special case except for CURRENT_TIMESTAMP,
!(s.starts_with('(') && s.ends_with(')')) might break behavior.

If there is no such case, I should replace that condition as you suggest.

Thank you for your comments!

Copy link
Contributor

@jacek-prisma jacek-prisma Feb 24, 2026

Choose a reason for hiding this comment

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

What is the special case with CURRENT_TIMESTAMP?

Copy link
Author

@ken0x0a ken0x0a Feb 25, 2026

Choose a reason for hiding this comment

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

I thought something like following will be error, but it doesn't.
(I haven't tried before)

create table a(
  id integer primary key,
  dt integer not null default (CURRENT_TIME)
);

Copy link
Author

@ken0x0a ken0x0a Feb 25, 2026

Choose a reason for hiding this comment

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

or maybe I thought, the following is allowed (which is not)

create table a(
  dt integer not null default unixepoch('subsec')
);

I'm not sure what I was thinking 🤔

Copy link
Author

@ken0x0a ken0x0a Feb 25, 2026

Choose a reason for hiding this comment

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

Fixed. Thank you!

format!("({})", s).into()
} else {
s.into()
}
}
Comment on lines +273 to +283
Copy link

@coderabbitai coderabbitai bot Feb 25, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Edge case: wrap_in_parentheses can be fooled by non-matching outer parentheses.

The check s.starts_with('(') && s.ends_with(')') does not verify that the first ( and last ) are actually a matched pair. For example, a default expression like (a) + (b) would pass the check and not be wrapped, producing an invalid result.

In practice this may be unlikely for SQLite defaults reaching the db_generated path, but a more robust approach would verify the outer parentheses are balanced:

🛡️ Proposed robust implementation 
 fn wrap_in_parentheses(s: &str) -> Cow<'_, str> {
-    if !(s.starts_with('(') && s.ends_with(')')) {
+    if !is_wrapped_in_parentheses(s) {
         format!("({})", s).into()
     } else {
         s.into()
     }
 }
+
+/// Returns true only if the outermost characters are a matched pair of parentheses
+/// enclosing the entire expression.
+fn is_wrapped_in_parentheses(s: &str) -> bool {
+    if !(s.starts_with('(') && s.ends_with(')')) {
+        return false;
+    }
+    let mut depth = 0i32;
+    for (i, ch) in s.char_indices() {
+        match ch {
+            '(' => depth += 1,
+            ')' => {
+                depth -= 1;
+                // If depth hits 0 before the last character, the outer parens aren't wrapping the whole thing.
+                if depth == 0 && i < s.len() - 1 {
+                    return false;
+                }
+            }
+            _ => {}
+        }
+    }
+    depth == 0
+}

Copy link
Contributor

@jacek-prisma jacek-prisma Mar 3, 2026

Choose a reason for hiding this comment

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

This appears a legitimate issue with this code

Copy link

@coderabbitai coderabbitai bot Mar 3, 2026

Choose a reason for hiding this comment

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

@jacek-prisma, thank you for confirming! You're right—the current implementation can be fooled by cases like (a) + (b) where the outer characters are parentheses but don't form a matched pair wrapping the entire expression.

Would you like me to implement the more robust version that properly validates balanced parentheses? I can create the fix following the approach suggested in the review comment.

Task: Fix wrap_in_parentheses edge case

  • Add a new helper function is_wrapped_in_parentheses that checks if the outer parentheses are a matched pair wrapping the entire expression
  • Update wrap_in_parentheses to use the new helper
  • Add unit tests to verify the fix handles edge cases like (a) + (b)

💡 Use @coderabbitai plan to create a plan for code changes


async fn push_columns(
table_name: &str,
container_id: Either<TableId, ViewId>,
Expand Down Expand Up @@ -306,26 +318,26 @@ async fn push_columns(
Some(match &tpe.family {
ColumnTypeFamily::Int => match SqlSchemaDescriber::parse_int(&default_string) {
Some(int_value) => DefaultValue::value(int_value),
None => DefaultValue::db_generated(default_string),
None => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
ColumnTypeFamily::BigInt => match SqlSchemaDescriber::parse_big_int(&default_string) {
Some(int_value) => DefaultValue::value(int_value),
None => DefaultValue::db_generated(default_string),
None => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
ColumnTypeFamily::Float => match SqlSchemaDescriber::parse_float(&default_string) {
Some(float_value) => DefaultValue::value(float_value),
None => DefaultValue::db_generated(default_string),
None => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
ColumnTypeFamily::Decimal => match SqlSchemaDescriber::parse_float(&default_string) {
Some(float_value) => DefaultValue::value(float_value),
None => DefaultValue::db_generated(default_string),
None => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
ColumnTypeFamily::Boolean => match SqlSchemaDescriber::parse_int(&default_string) {
Some(PrismaValue::Int(1)) => DefaultValue::value(true),
Some(PrismaValue::Int(0)) => DefaultValue::value(false),
_ => match SqlSchemaDescriber::parse_bool(&default_string) {
Some(bool_value) => DefaultValue::value(bool_value),
None => DefaultValue::db_generated(default_string),
None => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
},
ColumnTypeFamily::String => {
Expand All @@ -335,14 +347,14 @@ async fn push_columns(
"current_timestamp" | "datetime(\'now\')" | "datetime(\'now\', \'localtime\')" => {
DefaultValue::now()
}
_ => DefaultValue::db_generated(default_string),
_ => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
},
ColumnTypeFamily::Json => DefaultValue::value(default_string),
ColumnTypeFamily::Binary => DefaultValue::db_generated(default_string),
ColumnTypeFamily::Uuid => DefaultValue::db_generated(default_string),
ColumnTypeFamily::Binary => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
ColumnTypeFamily::Uuid => DefaultValue::db_generated(wrap_in_parentheses(&default_string)),
ColumnTypeFamily::Enum(_) => DefaultValue::value(PrismaValue::Enum(default_string)),
ColumnTypeFamily::Udt(_) | ColumnTypeFamily::Unsupported(_) => {
DefaultValue::db_generated(default_string)
DefaultValue::db_generated(wrap_in_parentheses(&default_string))
}
})
}
Expand Down