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

Improve BaseConnection::getForeignKeyData() and Forge::addForeignKey() #6468

Merged
merged 41 commits into from
Sep 24, 2022

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Sep 1, 2022

  1. BaseConnection::getForeignKeyData()
    • Improves the method for retrieving foreign keys.
    • It aligns all DBMS to use the same naming convention.
    • It adds some more information such as delete, update, and match rules.
    • The returned dataset uses a new structure which uses one row per foreign key with an array of fields - useful for composite foreign keys.
  2. Forge::addForeignKey()

This is the first part of #6430

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@sclubricants sclubricants added enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer 4.3 labels Sep 1, 2022
@kenjis kenjis added the breaking change Pull requests that may break existing functionalities label Sep 2, 2022
@kenjis
Copy link
Member

kenjis commented Sep 2, 2022

The returned dataset uses a new structure which uses one row per foreign key with an array of fields

Can you add the structure in the user guide?

@kenjis
Copy link
Member

kenjis commented Sep 2, 2022

@sclubricants Thank you for this PR. This is easy to understand!

The new PHPStan reports new errors. We've fixed. Please rebase to get rid of the errors.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

There are a few GitHub Action check failures. Also please check.

@sclubricants
Copy link
Member Author

There are two PHPUnit / PHP 7.4 - MySQLi (pull_request) tests. Not sure why. Not sure why one of them is failing.

ErrorException: Undefined property: stdClass::$constraint_name

Connection::_foreignKeyData()

        $sql = '
                SELECT
                    tc.constraint_name,
                    ....
                    tc.table_name = ' . $this->escape($table);

        if (($query = $this->query($sql)) === false) {
            throw new DatabaseException(lang('Database.failGetForeignKeyData'));
        }

        $query   = $query->getResultObject();
        $indexes = [];

        foreach ($query as $row) {
            $indexes[$row->constraint_name]['constraint_name']       = $row->constraint_name;

The only way I can see this failing is if MySql isn't returning the fields in lower case. I had this problem with Oracle in upper case.

@sclubricants sclubricants requested a review from kenjis September 3, 2022 01:50
@kenjis
Copy link
Member

kenjis commented Sep 3, 2022

There are two PHPUnit / PHP 7.4 - MySQLi (pull_request) tests. Not sure why. Not sure why one of them is failing.

The last one is MySQL 8.0.

@kenjis
Copy link
Member

kenjis commented Sep 4, 2022

6bfd0be

Fix MySql _foreignKeyData() SQL upper case

Please write why you changed to uppercase in the commit message.

We can easily know that this commit changed the column names to uppercase,
but it is difficult to know why this change was needed from the commit.

@kenjis
Copy link
Member

kenjis commented Sep 4, 2022

1b03ee7

Using a prefix causes problems when using dropForeignKey().

What's the problems?

system/Database/Forge.php Outdated Show resolved Hide resolved
system/Database/Forge.php Outdated Show resolved Hide resolved
@sclubricants
Copy link
Member Author

What's the problems?

When you call dropForeignKey() the table parameter gets prefixed by table prefix. I mentioned it elsewhere here..

@sclubricants
Copy link
Member Author

sclubricants commented Sep 4, 2022

Please write why you changed to uppercase in the commit message.

Good question.. basically cause it worked. Its something I've never seen with MySql. MySql has always returned the same letter case as the sql is typed in my experience but for some reason the server you are running that test on returns upper case no matter what.

Perhaps there might be a configuration for this?

@kenjis
Copy link
Member

kenjis commented Sep 4, 2022

basically cause it worked.

It seems MySQL 8 changed the behavior.

what are the changes in mysql 8 result rowset case? - Stack Overflow
https://stackoverflow.com/questions/54538448/what-are-the-changes-in-mysql-8-result-rowset-case

@kenjis
Copy link
Member

kenjis commented Sep 4, 2022

When you call dropForeignKey() the table parameter gets prefixed by table prefix.

Oh, we can't drop any key that does not begin with the DB prefix!

$sql = sprintf(
(string) $this->dropConstraintStr,
$this->db->escapeIdentifiers($this->db->DBPrefix . $table),
$this->db->escapeIdentifiers($this->db->DBPrefix . $foreignName)
);

The DB prefix was added in:
fdb8538#diff-a07677ecb7c1a5d29e0d0a8e72f00b5b77b577508602ada4fb8c5c1e0c7c5916

@sclubricants
Copy link
Member Author

Oh, we can't drop any key that does not begin with the DB prefix!

We definitely want to have the prefix. Someone might have a different connection on same database with duplicates of tables but with different prefixes. This way we avoid name collision.

@kenjis
Copy link
Member

kenjis commented Sep 6, 2022

We definitely want to have the prefix. Someone might have a different connection on same database with duplicates of tables but with different prefixes. This way we avoid name collision.

Do we need the prefix for foreign key name?

system/Database/Forge.php Outdated Show resolved Hide resolved
sclubricants and others added 21 commits September 24, 2022 14:08
On further review it appears that from 12.2 and on the identifier can be 128 bytes. However a user can set COMPATIBILITY to a lower version which could affect this.
OCI8 name suffix changed from _fk to _foreign
It seems its agreed that this is better being shorter.
Removed special naming for Oracle 12, Added the ability to set foreign key name, and added test.
Adding the db prefix was causing error when using custom foreign key names. Also refactored _processForeignKeys() to reduce duplicated code.
Kept one test to test default foreign key names - the rest set their own names. Removed the extra test to test setting names since this is done in other tests.
@kenjis kenjis removed the stale Pull requests with conflicts label Sep 24, 2022
@kenjis kenjis merged commit f875417 into codeigniter4:4.3 Sep 24, 2022
@kenjis
Copy link
Member

kenjis commented Sep 24, 2022

@sclubricants @michalsn Thank you!
This is a great improvement.

@kenjis kenjis changed the title Improve Connection::_foreignKeyData() Improve BaseConnection::getForeignKeyData() and Forge::addForeignKey() Sep 24, 2022
@MGatner
Copy link
Member

MGatner commented Sep 25, 2022

@kenjis said it best! Thanks to everyone for the work on this 🥳

@sclubricants sclubricants deleted the ForeignKeysFix branch September 25, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 breaking change Pull requests that may break existing functionalities database Issues or pull requests that affect the database layer enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants