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

fix: EXPOSED-701 [Oracle] Insert into table using only database default values fails #2371

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bog-walk
Copy link
Member

Description

Summary of the change: Oracle insert statement now uses valid syntax for inserting only default values.

Detailed description:

  • Why:

Oracle currently generates a statement like INSERT ... DEFAULT VALUES if an insert operation is performed with no provided column set, like Table.insert { }.
This was not caught because there is actually no test for insertion of only truly database-side defaults; all tests either insert at minimum 1 column, an expression, or implicit auto-inc value for example.

The above syntax causes java.sql.SQLSyntaxErrorException: ORA-00926: missing VALUES keyword because it is not supported. More details here

  • How: Oracle no longer relies solely on the default FunctionProvider.insert() implementation. It uses the INSERT ... VALUES(DEFAULT, ...) syntax if a defaults-only expression is detected.

Type of Change

Please mark the relevant options with an "X":

  • Bug fix

Affected databases:

  • Oracle

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)

Related Issues

EXPOSED-701

@bog-walk bog-walk requested a review from obabichevjb January 24, 2025 02:06
Comment on lines 366 to 380
override fun insert(
ignore: Boolean,
table: Table,
columns: List<Column<*>>,
expr: String,
transaction: Transaction
): String {
val def = super.insert(ignore, table, columns, expr, transaction)
return if (def.endsWith(" $DEFAULT_VALUE_EXPRESSION")) {
val oracleDefaults = "VALUES(DEFAULT${", DEFAULT".repeat(table.columns.lastIndex)})"
"${def.removeSuffix(DEFAULT_VALUE_EXPRESSION)}$oracleDefaults"
} else {
def
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Alternatively, if the string checks seem error-prone, a new branch dependent on currentDialect could be added to the default insert() implementation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a good question. OracleFunctionProvider extends FunctionProvider, so it doesn't look too good to add checks for dialect inside base FunctionProvider class.

The current check looks a bit fragile due to string check, but test should save us from regression.

I didn't check it, but looking at the check columns.isNotEmpty() -> columns to expr, could it be reasonable to check for columns.isNotEmpty() inside OracleFunctionProvider::insert(), and if columns are empty pass "${def.removeSuffix(DEFAULT_VALUE_EXPRESSION)}$oracleDefaults" instead of expression?

I feel like expr could be non empty, and it will not work, but I don't understand now what could be in expr if columns are empty...

…lt values fails

Oracle currently generates a statement like `INSERT ... DEFAULT VALUES` if an insert
operation is performed with no provided column set. This fails because that syntax
is not supported.

This switches to the `INSERT ... VALUES(DEFAULT, ...)` syntax instead and includes
a test for this case.
…lt values fails

- Refactor to include dialect check & logic in default insert()
@bog-walk bog-walk force-pushed the bog-walk/fix-oracle-default-insert branch from d395aa1 to f670fc8 Compare January 28, 2025 23:41
@bog-walk bog-walk requested review from obabichevjb and removed request for obabichevjb January 29, 2025 00:17
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.

2 participants