Fix identifier formatting for migrations when used with Oracledb.

[Kytech]

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]

coverage: 92.86% (+0.02%) from 92.842%
when pulling 523d91c on ITDeptUtahCountyGovernment:oracle-autoinc-trigger-formatting
into 3764ded on knex:master.

[baileymatthewr]

@Kytech : Could you assign some reviewers to this PR? I think it's ready to go.

[Kytech]

@Kytech : Could you assign some reviewers to this PR? I think it's ready to go.

Only the maintainers can assign reviewers, I don't have permission to do that. We just have to wait for the maintainers to triage PRs, but I can open an issue and link it to this PR to increase its visibility.

[baileymatthewr]

@OlivierCavadenti : this passes the tests and increases the coverage according to coveralls. Would you mind reviewing this one?

[Kytech]

Looks like the failing tests are unrelated to these changes. They're all in postgres and nothing in this PR touched any of the postgres codebase. I'll take some time a bit later on to see if there were any unexpected side-effects that oddly impacted those other tests, but it looks like a different commit pulled from master may have caused the failure looking at check statuses on the master branch.

[baileymatthewr]

@Kytech : The following was causing the Linting to fail:

TypeError: (0 , utils_1.installAllTypeScriptVersions) is not a function

That should be a pretty quick to fix.

[Kytech]

TypeError: (0 , utils_1.installAllTypeScriptVersions) is not a function

@baileymatthewr That error is not from any code in the knex repo. That's an error in the code for their linter package. This linting error occurs on Knex's main branch as well. Fixing an external linter dependency is out of scope for this PR. As long as we don't have any CI failures that are not in the main branch, the maintainers won't let that block merging, based on previous PRs. They just haven't seen this PR yet.

[Kytech]

This PR is superseded by: #6058

I no longer work for the organization that owns the repo this PR was originally created from and have moved the remaining work on this PR to a fork I maintain on my personal account.