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

Correct comparison of defaults for String type columns in PostgreSQL #1589

Merged

Conversation

AlexeySoshin
Copy link
Contributor

@AlexeySoshin AlexeySoshin commented Sep 5, 2022

Fixes #1587

Stripping the first single quote in PostgreSQL doesn't seem like a good strategy, since 'B'::text is a valid default value.

Branching with Text/Varchar seems necessary, because the default expression is always of TextColumnType, as it is mapped from String literal:

fun stringLiteral(value: String): LiteralOp<String> = LiteralOp(TextColumnType(), value)

@AlexeySoshin AlexeySoshin marked this pull request as ready for review September 5, 2022 19:13
@@ -172,6 +172,7 @@ class JdbcDatabaseMetadataImpl(database: String, val metadata: DatabaseMetaData)
is SQLServerDialect -> defaultValue.trim('(', ')', '\'')
is OracleDialect -> defaultValue.trim().trim('\'')
is MysqlDialect -> defaultValue.substringAfter("b'").trim('\'').trim()
is PostgreSQLDialect -> defaultValue.trim()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like trim here and in default branch are wrong. With it it's impossible to set default value with whitespaces (like " ").
Could you please remove the trim and adjust your test by replacing empty string with whitespaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out, I'll add another test to reproduce and will fix this.

val text = text("text_column").default("")
}

// MySQL doesn't support default values on text columns, hence excluded
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tapac Would appreciate your input on that

detekt.yml Outdated
@@ -1,7 +1,7 @@
# https://github.com/detekt/detekt/blob/main/detekt-core/src/main/resources/default-detekt-config.yml

build:
maxIssues: 52
maxIssues: 200
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tapac
I raised this as the current error count is 143, and I think Detekt errors should be addressed in a different PR

Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask you to merge master into the branch? Detekt issue was solved there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tapac thank you, I'll do that

@Tapac Tapac merged commit 93033f4 into JetBrains:master Nov 14, 2022
Tapac added a commit that referenced this pull request Nov 14, 2022
Tapac added a commit that referenced this pull request Nov 14, 2022
…1589) / Oracle threats '' as NULL, PSQL fails on comparing non-string defaults
Tapac added a commit that referenced this pull request Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants