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

test: Fix failing Oracle tests in exposed-tests #1831

Merged
merged 1 commit into from
Aug 11, 2023

Conversation

bog-walk
Copy link
Member

The following tests fail when run on Oracle:

CreateMissingTablesAndColumnsTests/testCamelCaseForeignKeyCreation()

Fails with:

ORA-02275: such a referential constraint already exists in the table.
Statement(s): ALTER TABLE RECEIPTS ADD CONSTRAINT FK_RECEIPTS_TRACENUMBER__TRACENUMBER FOREIGN KEY (TRACENUMBER) REFERENCES TMPORDERS(TRACENUMBER)

This happens because Oracle metadata (via getImportedKeys()) only returns foreign keys that reference primary keys of the referenced table (documentation), so an empty map is being retrieved, which triggers an unnecessary alter statement.

This test passes in other DB as their metadata seems to be more lenient, but the metadata being the issue can be confirmed by changing the table mapping. For example, if the referenced table column traceNumber is made to be the primary key instead of a unique index, getImportedKeys() returns a map with the FK and the test passes.

This is a known issue with Oracle JDBC, so the test now excludes it:


EntityTests/preloadOptionalReferencesOnASizedIterable()

Fails on the first assertNotNull() because testCache() is passed the argument school1.id = 0, which does not exist. The new entity school1 is actually cached with id.value = 1, but the argument passed does not reflect this unless the entity is first flushed.

This seems to be Oracle specific because all reference syntax and all breakpoints match between Oracle and other DB used as comparison, until the point where testCache(school1.id) is invoked (other DB correctly return id.value = 1).

It may be related, but another way to have the test pass is to:

  • Have the second School entity, school2, provide an argument for secondaryRegion instead of using the default null. So this may actually be an Oracle issue specific to nullable foreign key constraints, but no documentation states that the latter is not possible.

Also, this test has a consistent and significant lag between the creation of the second and third entities (where it attempts to store a value to the nullable FK constraint).

The entity is flushed to allow the test to continue, but Oracle could be excluded if the underlying issue requires more investigation.

The following tests fail when run on Oracle:

CreateMissingTablesAndColumnsTests/testCamelCaseForeignKeyCreation()

Fails with:
ORA-02275: such a referential constraint already exists in the table.

Because Oracle metadata (via getImportedKeys()) only returns foreign keys that
reference primary keys of the referenced table.

The test now excludes Oracle.

EntityTests/preloadOptionalReferencesOnASizedIterable()

Fails on the first assertNotNull() because testCache() is unable to find
school1.id = 0, which does not exist. The entity school1 is cached with its correct
id.value = 1, but an incorrect value is passed as an argument unless the entity
is first flushed after creation.
Comment on lines 520 to +524
val traceNumber = reference("traceNumber", ordersTable.traceNumber)
}

withDb {
// Oracle metadata only returns foreign keys that reference primary keys
withDb(excludeSettings = listOf(TestDB.ORACLE)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative to excluding Oracle would be to change the mapping of the test tables so that a primary key is referenced instead of a unique index:

val ordersTable = object : Table("tmporders") {
    val traceNumber = char("traceNumber", 10)
    override val primaryKey = PrimaryKey(traceNumber)
}
val receiptsTable = object : IntIdTable("receipts") {
    val traceNumber = reference("traceNumber", ordersTable.traceNumber)
}

This would still retain the original intention of the unit test, I think, which seems more about column naming than whether the column is a primary key or a unique index.

@bog-walk bog-walk requested review from e5l and joc-a August 10, 2023 18:08
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

Nice! Lgtm

@@ -844,6 +844,9 @@ class EntityTests : DatabaseTestsBase() {
name = "Eton"
region = region1
secondaryRegion = region2
}.apply {
// otherwise Oracle provides school1.id = 0 to testCache(), which returns null
if (currentDialectTest is OracleDialect) flush()
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. What does flush mean? Maybe I missed it, but I couldn't find anything about it in the documentation.
  2. Do you think this test reflects real-life usage? If we expect users to use Exposed like this with Oracle, what do you think of adding this to the documentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

When new() is used, it creates an entity but then just queues it in a cache for insertion (even though SQL statements are logged). flush() manually takes the entities in the cache (as well as any entities that have been queued for update) and sends the values to the database. This happens under-the-hood at the end of a transaction or before/during certain other functions that force access to the entities.

I don't think this is a real-life scenario for any users, using testCache(), as anything that would access the created entity would trigger a flush. For example, if the apply {} block was removed and I did something like findById() or even just a println(school1.id), the test would pass.

@bog-walk bog-walk merged commit 5f06400 into main Aug 11, 2023
3 checks passed
@bog-walk bog-walk deleted the bog-walk/exclude-oracle-from-tests branch August 11, 2023 12:31
saral pushed a commit to saral/Exposed that referenced this pull request Oct 3, 2023
The following tests fail when run on Oracle:

CreateMissingTablesAndColumnsTests/testCamelCaseForeignKeyCreation()

Fails with:
ORA-02275: such a referential constraint already exists in the table.

Because Oracle metadata (via getImportedKeys()) only returns foreign keys that
reference primary keys of the referenced table.

The test now excludes Oracle.

EntityTests/preloadOptionalReferencesOnASizedIterable()

Fails on the first assertNotNull() because testCache() is unable to find
school1.id = 0, which does not exist. The entity school1 is cached with its correct
id.value = 1, but an incorrect value is passed as an argument unless the entity
is first flushed after creation.
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.

3 participants