fix: Inaccurate drop database statement in Oracle #1807
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Failing test shows that the override for dropDatabase() in OracleDialect is wrong, so this has been replaced with the correct statement. Failing test would then require privileges to drop the pluggable database from the root container level, which are not accessible, so it has been excluded.
Change test names casing and exception name
e5l
requested changes
Jul 31, 2023
| @@ -318,7 +318,7 @@ open class OracleDialect : VendorDialect(dialectName, OracleDataTypeProvider, Or | |||
|
|
|||
| override fun createDatabase(name: String): String = "CREATE DATABASE ${name.inProperCase()}" | |||
|
|
|||
| override fun dropDatabase(name: String): String = "DROP DATABASE ${name.inProperCase()}" | |||
| override fun dropDatabase(name: String): String = "DROP DATABASE" | |||
There was a problem hiding this comment.
Could you tell me why don't we need the name in drop statement?
There was a problem hiding this comment.
@e5l Documentation shows that syntax should not have the database name in the drop statement.
This can be confirmed in the test if SchemaUtils.dropDatabase(dbName) is pulled out of the try block (or if the cause.cause is logged in the catch block) as it fails with this exception:
java.sql.SQLSyntaxErrorException: ORA-00933: SQL command not properly ended.
Statement(s): DROP DATABASE JETBRAINS
Removing the name resolves this exception (but then leads to the next exception - ORA-65040: operation not allowed from within a pluggable database)
e5l
approved these changes
Jul 31, 2023
saral
pushed a commit
to saral/Exposed
that referenced
this pull request
Oct 3, 2023
* fix: Inaccurate drop database statement in Oracle Failing test shows that the override for dropDatabase() in OracleDialect is wrong, so this has been replaced with the correct statement. Failing test would then require privileges to drop the pluggable database from the root container level, which are not accessible, so it has been excluded.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following test fails when run locally with Oracle:
CreateDatabaseTest/'create database test'()
The first failure
ORA-01501: CREATE DATABASE failed, ORA-01100: database already mountedoccurs because the create statement is executed while a database already exists. This occurs because thedropDatabase()statement executed in the try block fails as the syntax is not correct.Once the syntax is fixed, the test still fails with error
ORA-65040: operation not allowed from within a pluggable databasebecause certain operations, including dropping the DB, need to be performed only at the root container level.Attempting to fix this with
exec("ALTER SESSION SET CONTAINER=CDB\$ROOT")then fails with errorORA-01031: insufficient privileges. This is in spite of the test container connection being setup usingsys as sysdbaand granting all privileges to user.The documentation confirms that dropping a database requires certain conditions:
Attempting to execute variants of the following statements doesn't work either (most likely because they are SQL*PLUS CLI commands):
Oracle has been excluded from this test.