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

SQLite3 Connection getIndexData() #6221

Merged
merged 8 commits into from
Jul 13, 2022

Conversation

sclubricants
Copy link
Member

@sclubricants sclubricants commented Jul 1, 2022

Supersedes #6207

This is a rewrite of system/Database/SQLite3/Connection::_indexData().

SQLite doesn't show indexes on tables created with INTEGER PRIMARY KEY AUTO_INCREMENT. This PR looks for primary keys that don't appear in the normal index lookup and adds it to the return of the method. It also adds the key type: Primary, Unique, or Index.

In the test database creation of the users table the current implementation won't return any index.

        $this->forge->addField([
            'id'         => ['type' => 'INTEGER', 'constraint' => 3, 'auto_increment' => true],
            'name'       => ['type' => 'VARCHAR', 'constraint' => 80],
            'email'      => ['type' => 'VARCHAR', 'constraint' => 100],
            'country'    => ['type' => 'VARCHAR', 'constraint' => 40],
            'created_at' => ['type' => 'DATETIME', 'null' => true],
            'updated_at' => ['type' => 'DATETIME', 'null' => true],
            'deleted_at' => ['type' => 'DATETIME', 'null' => true],
        ])->addKey('id', true)->createTable('user', true); 

However if you wrote it like the following it would show up in the indexes:

        $this->forge->addField([
            'id'         => ['type' => 'INTEGER', 'constraint' => 3],
            'name'       => ['type' => 'VARCHAR', 'constraint' => 80],
            'email'      => ['type' => 'VARCHAR', 'constraint' => 100],
            'country'    => ['type' => 'VARCHAR', 'constraint' => 40],
            'created_at' => ['type' => 'DATETIME', 'null' => true],
            'updated_at' => ['type' => 'DATETIME', 'null' => true],
            'deleted_at' => ['type' => 'DATETIME', 'null' => true],
        ])->addKey('id', true)->createTable('user', true); 

With this PR both cases will return indexes for primary key.

The need is to get a comprehensive list of database constraints. This will be useful in adding upsert() to the database.

Furthermore, this method doesn't use nested queries like the current implementation and requires only one database call.

Its important to note that something like the following would likely cause an error:

        $keys = $db->getIndexData('tablename');
        
        foreach ($keys as $key) {
            // if ($key->name !== 'PRIMARY')
            $forge->dropKey('tablename', $key->name);            
        }

SQLite's handling of things the way they do creates a pseudo index but its not an index for the purposes of calling database commands. If you check that the index isn't name PRIMARY then the rest should be ok.

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

@kenjis kenjis added 4.3 enhancement PRs that improve existing functionalities database Issues or pull requests that affect the database layer labels Jul 1, 2022
@sclubricants
Copy link
Member Author

Got some work to do in CodeIgniter4\system\Database\SQLite3\Table

Line 111

 $this->keys = array_merge($this->keys, $this->formatKeys($this->db->getIndexData($table)));

@sclubricants
Copy link
Member Author

Adding

SELECT `table` || '_' || `from` || '_foreign' as indexname, `from` as fieldname, 'FOREIGN' as indextype FROM pragma_foreign_key_list('" . trim($table, '`') . "') UNION ALL

to _indexData query will retrieve foreign keys as well. This may be a step to far though it seems all the other databases return these with indexes. There is a separate method that will retrieve these.

@sclubricants
Copy link
Member Author

@kenjis @MGatner Need anything else with this?

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

The PR looks good, very nice tests. I really am not qualified to decide if this is a better approach to SQLite, but it seems like you are mostly expanding on what we already have so I'll give a tentative "approve". Need somebody else to chime in as well.

@kenjis kenjis added the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 9, 2022
@kenjis
Copy link
Member

kenjis commented Jul 9, 2022

@sclubricants Thank you for the PR.
This is an enhancement, so please add something in the changelog.
https://github.com/codeigniter4/CodeIgniter4/blob/4.3/user_guide_src/source/changelogs/v4.3.0.rst#enhancements

@kenjis
Copy link
Member

kenjis commented Jul 9, 2022

It may be kind to add the note in https://codeigniter4.github.io/CodeIgniter4/database/metadata.html#db-getindexdata

Its important to note that something like the following would likely cause an error:

@kenjis
Copy link
Member

kenjis commented Jul 9, 2022

In the unlikely case such as:
...
The id column doesn't get an index but the email column does. The way I wrote it the primary key index will assume the name of the email index but include both fields and have a type PRIMARY

What do you mean? I got the array when specifying addKey(['id', 'email'], true):

array (1) [
    0 => stdClass (3) (
        public 'indexname' -> string (7) "PRIMARY"
        public 'fieldname' -> string (2) "id"
        public 'indextype' -> string (7) "PRIMARY"
    )
]

@sclubricants
Copy link
Member Author

What do you mean? I got the array when specifying addKey(['id', 'email'], true):

I'm not sure where I got that idea. I thought it was something that I tested but somehow I'm unable to replicate it now. After looking at the code its impossible to do because if there is an auto increment column then only the one column will be used and any others discarded. This is why the email doesn't show up in PK. I tried it in SQL and it throws an error if you try to set multiple PK with an AUTOINCREMENT column.

So disregard that.

@sclubricants
Copy link
Member Author

@kenjis User Guide and Change Log updated.

@kenjis
Copy link
Member

kenjis commented Jul 13, 2022

Before:

array (2) [
    'testuser_email' => stdClass (2) (
        public 'name' -> string (14) "testuser_email"
        public 'fields' -> array (1) [
            0 => string (5) "email"
        ]
    )
    'testuser_country' => stdClass (2) (
        public 'name' -> string (16) "testuser_country"
        public 'fields' -> array (1) [
            0 => string (7) "country"
        ]
    )
]

After:

array (3) [
    'PRIMARY' => stdClass (3) (
        public 'name' -> string (7) "PRIMARY"
        public 'fields' -> array (1) [
            0 => string (2) "id"
        ]
        public 'type' -> string (7) "PRIMARY"
    )
    'testuser_email' => stdClass (3) (
        public 'name' -> string (14) "testuser_email"
        public 'fields' -> array (1) [
            0 => string (5) "email"
        ]
        public 'type' -> string (6) "UNIQUE"
    )
    'testuser_country' => stdClass (3) (
        public 'name' -> string (16) "testuser_country"
        public 'fields' -> array (1) [
            0 => string (7) "country"
        ]
        public 'type' -> string (5) "INDEX"
    )
]

sclubricants and others added 2 commits July 13, 2022 10:21
Co-authored-by: kenjis <kenji.uui@gmail.com>
Co-authored-by: kenjis <kenji.uui@gmail.com>
@kenjis kenjis removed the docs needed Pull requests needing documentation write-ups and/or revisions. label Jul 13, 2022
@kenjis kenjis merged commit 9755623 into codeigniter4:4.3 Jul 13, 2022
@sclubricants sclubricants deleted the SQLiteConnectiongetIndexData branch July 15, 2022 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.3 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.

3 participants