fix: prevent breaking default expr in SQLite#5748
fix: prevent breaking default expr in SQLite#5748ken0x0a wants to merge 5 commits intoprisma:mainfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This PR fixes issue #29064 where SQLite's PRAGMA table_info removes parentheses from default value expressions, causing schema drift on re-introspection. A wrap_in_parentheses helper function is added to restore parentheses when generating Prisma schemas.
Changes:
- Added
wrap_in_parenthesesfunction to restore parentheses removed by SQLite's PRAGMA - Applied the function to db_generated defaults for Int, BigInt, Float, Decimal, Boolean, and DateTime types
- Added test case with complex default expression to verify the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| schema-engine/sql-schema-describer/src/sqlite.rs | Implements wrap_in_parentheses helper and applies it to various column type default values |
| schema-engine/sql-introspection-tests/tests/re_introspection/sqlite.rs | Adds test case with CAST(unixepoch('subsec') * 1000 AS INTEGER) default to verify parentheses are preserved |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ => DefaultValue::db_generated(wrap_in_parentheses(&default_string)), | ||
| }, | ||
| ColumnTypeFamily::Json => DefaultValue::value(default_string), | ||
| ColumnTypeFamily::Binary => DefaultValue::db_generated(default_string), |
There was a problem hiding this comment.
The wrap_in_parentheses function is not applied to the Binary column type, but it is applied to other column types (Int, BigInt, Float, Decimal, Boolean, DateTime). Since the issue being fixed is that SQLite's PRAGMA table_info removes parentheses from default values for all types, this should likely be applied consistently to Binary as well for consistency, unless there's a specific reason Binary defaults don't need parentheses wrapping.
| ColumnTypeFamily::Binary => DefaultValue::db_generated(default_string), | |
| ColumnTypeFamily::Binary => DefaultValue::db_generated(wrap_in_parentheses(&default_string)), |
There was a problem hiding this comment.
@ken0x0a I think this comment is valid and the function should be applied for all types
| /// 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[0..1] != *"(" && s[s.len() - 1..] == *")" { |
There was a problem hiding this comment.
The condition logic is incorrect. This checks if the first character is NOT '(' AND the last character IS ')', which is the opposite of what should be checked. This will wrap strings that end with ')' but don't start with '(', and will not wrap strings that need wrapping.
The condition should be: !s.starts_with('(') || !s.ends_with(')') to check if either parenthesis is missing, or more likely: !(s.starts_with('(') && s.ends_with(')')) to check if the string is not already fully wrapped.
| if s[0..1] != *"(" && s[s.len() - 1..] == *")" { | |
| if !(s.starts_with('(') && s.ends_with(')')) { |
There was a problem hiding this comment.
@ken0x0a indeed, the condition should probably be !(s.starts_with('(') && s.ends_with(')'))
|
The most important thing I must confess is, If this doesn't fix it, could anyone let me know how to fix it... |
Merging this PR will not alter performance
Comparing 
Footnotes
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| /// 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(')') { |
There was a problem hiding this comment.
I think this is meant to be
!(s.starts_with('(') && s.ends_with(')'))
There was a problem hiding this comment.
No, it's not.
We need to wrap with parentheses as PRAGMA table_info removes parentheses from default values.
There was a problem hiding this comment.
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 ).
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
What is the special case with CURRENT_TIMESTAMP?
There was a problem hiding this comment.
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)
);There was a problem hiding this comment.
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 🤔
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdded a new Changes
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
schema-engine/sql-introspection-tests/tests/re_introspection/sqlite.rsschema-engine/sql-schema-describer/src/sqlite.rs
| t.add_column( | ||
| "createdAt", | ||
| types::custom("INTEGER NOT NULL DEFAULT (CAST(unixepoch('subsec') * 1000 AS INTEGER))"), | ||
| ); |
There was a problem hiding this comment.
🛠️ 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:
- Make the regression coverage self-documenting and easy to find.
- 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
There was a problem hiding this comment.
This comment is valid, it might make sense to introduce a similar test that just covers this
There was a problem hiding this comment.
@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:
- Sets up a table with a column using a complex default expression (like the
unixepochexample) - Performs introspection to verify the default is correctly wrapped in parentheses
- 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_parenthesesin 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
| /// 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(')')) { | ||
| format!("({})", s).into() | ||
| } else { | ||
| s.into() | ||
| } | ||
| } |
There was a problem hiding this comment.
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
+}There was a problem hiding this comment.
This appears a legitimate issue with this code
There was a problem hiding this comment.
@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_parenthesesthat checks if the outer parentheses are a matched pair wrapping the entire expression - Update
wrap_in_parenthesesto 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
This PR is intended to fix:
prisma migrate devredefine table endlessly prisma#29064Summary by CodeRabbit
New Features
Bug Fixes
Tests