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 identifier formatting for migrations when used with Oracledb #6058

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

Kytech
Copy link
Contributor

@Kytech Kytech commented Apr 17, 2024

Presently, knex encounters issues in Oracle database when certain migration commands are executed when a custom wrapIdentifier option is specified in the knexfile - particularly when this option is used to normalize casing and formatting of database objects (ex. camelCase to UPPER_SNAKE_CASE). The cause of the issue is inconsistent usage of identifier formatting with formatter.wrap() throughout the oracle schema query compilers, where some parts of the code are formatting identifiers, and others are not in places that alter the same resource. This is problematic as Oracle treats any identifier surrounded in quotes as case-sensitive and knex routinely quotes all identifiers in its queries.

A prime example of this is the table and column renaming schema builder methods, which would not format identifiers that were passed-in to these methods. This contrasts with how table and column names are formatted on initial creation. This inconsistency sometimes results in failing queries when the exact same name used during creation and renaming of a table or column were used if the name passed to the query builder was later formatted by wrapIdentifer in a way that would alter the casing of the name. An example of such a scenario would look like this:

// knex created from knexfile w/wrapIdentifier configured to convert lower_snake and camelCase to UPPER_SNAKE_CASE
const knex = require('knexInstance');

await knex.scema.createTable('exampleTable', (table) => {  // Resultant table name would be "EXAMPLE_TABLE"
    // table builder statements
});

await knex.schema.renameTable('exampleTable', 'newTable');
// Fails with error from Oracle stating that table "exampleTable" does not exist.

Another instance of inconsistent formatting was causing issues with the dropTableIfExists() method as well. The table name would be successfully formatted and dropped, though the auto-generated sequence for a .increments() id column would fail with an error. This is because knex always generates these auto-generated sequences in all lowercase regardless of what is specified in wrapIdentifier, while the dropSequenceIfExists() schema builder compiler for Oracle would always format the sequence name. Building off of the previous example, the following statement would also fail:

// auto-generated sequence for exampleTable is named "example_table_seq"

await knex.schema.dropTableIfExists('exampleTable');
// drops "EXAMPLE_TABLE", attempts to drop "EXAMPLE_TABLE_SEQ"
// Oracle throws an error because "EXAMPLE_TABLE_SEQ" does not exist

This was fixed by altering the code path for related sequence deletion to not format the sequence name on deletion. I took this approach instead of removing formatting in dropSequenceIfExists() directly as previous changelogs indicated that this may have been previously, or is intended to be used if or when a public API for sequence creation in Oracle is added to knex. If this is not the case, I can refactor that to eliminate some dead code since dropSequenceIfExists() is not presently in use by any other part of the codebase (either by removing that function or prefixing it with _ to indicate that it is intended for private use). This approach was taken instead of formatting auto-created triggers going forward to avoid breaking migration commands that manipulate these triggers and sequences (as new implementations would always format these names where prior versions would have always lower_snake_case'd the trigger and sequence name).

I had to make several adjustments to some of the existing unit tests for oracle since the code now calls the wrapIdentifier function an additional time during the affected tests. I also added several new unit tests to check that wrapIdentifier is called properly in the schema builder functions that were affected by the inconsistent formatting issues.

Fixes #5801

@coveralls
Copy link

coveralls commented Apr 17, 2024

Coverage Status

coverage: 92.86% (+0.02%) from 92.843%
when pulling 38919b3 on Kytech:oracle-autoinc-trigger-formatting
into 9659a20 on knex:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants