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

Add db:create and db:drop support for MariaDB #934

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pbstriker38
Copy link

@pbstriker38 pbstriker38 commented Dec 16, 2020

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test pass with this change (including linting)?
    No, but linting issues are unrelated to my change.
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
    I have db drop and create tests that pass. Some of the other pre-existing tests do not pass locally for me.
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

Description of change

Adds support for db:create and db:drop for MariaDB. This uses the same code block as MySQL since they are compatible.

I can push up the tests if you want. It's just an extra if (Support.dialectIsMariaDB()) { in the db-create and db-drop test files.

Closes #1236

@WikiRik
Copy link
Member

WikiRik commented Nov 9, 2021

Hi, thanks for submitting this PR. And our apologies for the late response. Are you still willing to add some tests to this?

@msanguineti
Copy link

msanguineti commented Dec 6, 2021

I was trying to fix this problem but I have found inconsistencies in how sequelize returns the list of tables for a database schema. These inconsistencies cause failures in tests which then needs special handling making them a bit brittle.

sequelize returns an array of strings with table names for mysql, pgsql and sqlite while it returns an array of objects for mariadb and mssql.

// `mysql`, `pgsql` and `sqlite`
showTableNames() -> ['tableA', 'tableB', ...]

// `mariadb` and `mssql`
showTableNames() -> [{tableName: 'tableA', schema: 'schemaX}, {tableName: 'tableB', schema: 'scehmaX}, ...]

So, I could write tests to handle this inconsistencies thus making the cli handle create/drop for mariadb, but I am wondering if this issue should really be fixed in sequelize itself by normalising the API across all the dialects. Also, I am wondering how tests for mssql are passing. I do not have a MSSQL server I can test against so my doubts remain.

EDIT:

I have tried to run the tests against a MSSQL database using the docker image provided by Microsoft and indeed the tests are failing due to the same reason that the returned list of table names is a list of objects and not a simple list of strings (names).

@WikiRik
Copy link
Member

WikiRik commented Dec 6, 2021

I was trying to fix this problem but I have found inconsistencies in how sequelize returns the list of tables for a database schema. These inconsistencies cause failures in tests which then needs special handling making them a bit brittle.

sequelize returns an array of strings with table names for mysql, pgsql and sqlite while it returns an array of objects for mariadb and mssql.

// `mysql`, `pgsql` and `sqlite`
showTableNames() -> ['tableA', 'tableB', ...]

// `mariadb` and `mssql`
showTableNames() -> [{tableName: 'tableA', schema: 'schemaX}, {tableName: 'tableB', schema: 'scehmaX}, ...]

So, I could write tests to handle this inconsistencies thus making the cli handle create/drop for mariadb, but I am wondering if this issue should really be fixed in sequelize itself by normalising the API across all the dialects. Also, I am wondering how tests for mssql are passing. I do not have a MSSQL server I can test against so my doubts remain.

EDIT:

I have tried to run the tests against a MSSQL database using the docker image provided by Microsoft and indeed the tests are failing due to the same reason that the returned list of table names is a list of objects and not a simple list of strings (names).

Hi. Thanks for looking into this!
It would indeed be good if it is normalized in sequelize itself, but if a dialect does not support schemas it would be prettier to return only the table names. So I don't think a fix like this would be the best solution. I think we should just accommodate for both options. I will work on update the CI to include the same dialects as sequelize itself and then maybe we can work together on fixing the failing tests?

@ZeroExistence
Copy link

Hi, just want to check if there will be an update on reviewing this mariadb support for create/drop action. Thank you.

@boadadf
Copy link

boadadf commented Oct 18, 2023

For God's sake, someone please, accept this MR.
It's just a new case to the switch. It can't break anything since the syntax for mysql and mariadb is the same!

Furthermore
db-create.test.js
Tests indeed both (mysql and mariadb) since:
if (Support.dialectIsMySQL()) {

Refers to index.js:
Support.dialectIsMySQL: function (strict) {
that returns
return ['mysql', 'mariadb'].indexOf(envDialect) !== -1;
Which is true in this case.

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

Successfully merging this pull request may close these issues.

CLI does not support db:create and db:drop for MariaDB
5 participants