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

Added option to pass indexType for MySQL dialect #2890

Merged
merged 3 commits into from Nov 10, 2018

Conversation

Projects
None yet
2 participants
@douglasjunior
Copy link
Contributor

douglasjunior commented Nov 5, 2018

I followed the MySQL Reference.

CREATE [UNIQUE | FULLTEXT | SPATIAL] INDEX index_name
    [index_type]
    ON tbl_name (key_part,...)
    [index_option]
    [algorithm_option | lock_option] ...

key_part: {col_name [(length)] | (expr)} [ASC | DESC]

index_option:
    KEY_BLOCK_SIZE [=] value
  | index_type
  | WITH PARSER parser_name
  | COMMENT 'string'
  | {VISIBLE | INVISIBLE}

index_type:
    USING {BTREE | HASH}

algorithm_option:
    ALGORITHM [=] {DEFAULT | INPLACE | COPY}

lock_option:
    LOCK [=] {DEFAULT | NONE | SHARED | EXCLUSIVE}

No changes are required in types/knex.d.ts.

I'm not sure if I need to write tests for this, I could not find where the MySQL schema tests are written.

Steps to reproduce:

  1. Add my fork as dependency in package.json:
"knex": "git+https://github.com/douglasjunior/knex.git#custom"
  1. Create a migration file:
exports.up = (knex, Promise) => {
    return  knex.schema.createTable('my_table', table => {
        table.bigIncrements('id');

        table.string('my_long_text_column', 1000).notNullable();

        table.index('my_long_text_column', null, 'FULLTEXT'); // <= here
    });
};

exports.down = (knex, Promise) => {
    return knex.schema.dropTableIfExists('my_table');
};
  1. Run npx knex migrate:latest.

  2. See the generated schema:

CREATE TABLE `my_table` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  `my_long_text_column` varchar(1000) NOT NULL,
  PRIMARY KEY (`id`),
  FULLTEXT KEY `my_table_my_long_text_column_index` (`nome_original`)
) 

Related to #1840

@@ -194,14 +194,13 @@ assign(TableCompiler_MySQL.prototype, {
})
);
},
index(columns, indexName) {
index(columns, indexName, indexType) {

This comment has been minimized.

@kibertoad

kibertoad Nov 5, 2018

Collaborator

can you also add test for this? probably just a unit test for query generation would suffice

This comment has been minimized.

@kibertoad

kibertoad Nov 5, 2018

Collaborator

and there probably are existing tests for pg already

This comment has been minimized.

@douglasjunior

douglasjunior Nov 5, 2018

Contributor

The format of the tests is confusing to me. Many tests are failing and I do not know why. https://travis-ci.org/tgriesser/knex/jobs/450867352 (The tests fail even before the change)

I'll look at the postgre tests to see if I can understand.

This comment has been minimized.

@douglasjunior

douglasjunior Nov 5, 2018

Contributor

Done.

This comment has been minimized.

@kibertoad

kibertoad Nov 5, 2018

Collaborator
    it('test adding index', function() {
      tableSql = client
        .schemaBuilder()
        .table('users', function() {
          this.index(['foo', 'bar'], 'baz');
        })
        .toSQL();

      equal(1, tableSql.length);
      expect(tableSql[0].sql).to.equal(
        'alter table `users` add index `baz`(`foo`, `bar`)'
      );
    });

This is an example of a test failing. I assume structure of an index statement changed even when index type is not specified, which is actually probably not something that we want - maybe excessive space somewhere?..

This comment has been minimized.

@kibertoad

kibertoad Nov 5, 2018

Collaborator

@douglasjunior Feel free to ignore Oracle tests failing, they've been like that for a while already. But MySQL ones should be passing.

This comment has been minimized.

@douglasjunior

douglasjunior Nov 5, 2018

Contributor

Now I think I get it, please, could you check it again?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 5, 2018

@douglasjunior Thank you very much! Would you consider creating PR for https://github.com/knex/documentation to mention MySQL support for .index()?

@kibertoad

This comment has been minimized.

Copy link
Collaborator

kibertoad commented Nov 5, 2018

@tgriesser LGTM, what do you think?

@douglasjunior

This comment has been minimized.

Copy link
Contributor

douglasjunior commented Nov 5, 2018

@douglasjunior Thank you very much! Would you consider creating PR for https://github.com/knex/documentation to mention MySQL support for .index()?

Sure.

@douglasjunior

This comment has been minimized.

Copy link
Contributor

douglasjunior commented Nov 5, 2018

@kibertoad kibertoad added the ready label Nov 6, 2018

@kibertoad kibertoad merged commit 931c157 into tgriesser:master Nov 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 84.741%
Details

mwilliammyers added a commit to voxjar/knex that referenced this pull request Dec 12, 2018

Added option to pass indexType for MySQL dialect (tgriesser#2890)
* Added option to pass indexType for MySQL dialect.

* Added test for: test adding index with type

* Removing extra space when no type is provided.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment