Add support for schema() method on models in mysql dialect

[jeffleus]

Issue Description

Model has a schema() method that should allow multi-tenant design but it is not supported for the mysql dialect. The schema is prepended to the tableName with dot notation, `Schema.Table`, in the generated SQL. But for this to work with mysql it would need to wrap the schema and table separately in tick marks, `Schema`.`Table`.

Is your feature request related to a problem? Please describe.

This is frustrating because I would like to extend my sequelize-based back-end service to be multi-tenant. If sequelize was updated to support this, I could use a single (or pooled) db connection that would not need to change. I originally tried changing the connection using the beforeConnect() hook. But, this failed in the lambda environment because of container re-use and the connection pooling. With the schema change, I could connect each customer model to the correct schema w/one service and one connection pool. Then, I would get data isolation and have a db/schema for each customer on the MySQL RDS instance. Note, this appears to work correctly for the postgres dialect based on the documentation.

Describe the solution you'd like

I'd like to see the dialect for mysql support schema and output tableNames in the generated SQL as `schema`.`table`. I think it may be as simple as marking the dialect by adding a "schema: true" attribute to the supports property in the dialect's index.js file.

link to code source for dialect

MysqlDialect.prototype.supports = _.merge(_.cloneDeep(AbstractDialect.prototype.supports), {
//suggesting that the mysql dialect explicitly identify schemas as supported
  schemas: true, 
  'VALUES ()': true,
  //cut out remaining supports object for brevity in feature request
  ...
});

Why should this be in Sequelize

This works for the postgres dialect w/ the double quote ticks wrapping the schema and the table separately ( "schema"."table"). If the mysql dialect did this but replaced the double-quote with back ticks it should work.

Describe alternatives/workarounds you've considered

I tried to set the delimiter in the schema options to '.' as a workaround. However, the abstract query-generator and the quote helper strip these interior backticks from the tableName. I removed the "scrubbing code" in my local project and this worked. But this isn't a production worthy fix.

Additional context

I would be willing to generate a PR for this. My initial local testing for a single entity seemed to work. But, I would need some help in how to complete the necessary testing. This would be my first PR open source contribution, so I would like to tackle it. Just need a little help.

Issue Template Checklist

Is this issue dialect-specific?

• No. This issue is relevant to Sequelize as a whole.
• Yes. This issue only applies to the following dialect(s): mysql

Would you be willing to resolve this issue by submitting a Pull Request?

• Yes, I have the time and I know how to start.
• Yes, I have the time but I don't know how to start, I would need guidance.
• No, I don't have the time, although I believe I could do it if I had the time...
• No, I don't have the time and I wouldn't even know how to start.

[papb]

Hello, thanks for the detailed description. When you say it's a MySQL only issue, is it because this feature already exists for the other databases, or something else?

I would be willing to generate a PR for this.

Thanks! That's great!

This would be my first PR open source contribution, so I would like to tackle it. Just need a little help.

Nice. I shall help you 😬 I am a bit time constrained though, so I might have some delays between replies.

[papb]

The first thing I would like to ask:

• Please show a code snippet on how you currently try to do things and they don't work
• If your feature adds a new option, please show a code snippet on how things will be done after your feature is implemented

[jeffleus]

So I created a gist that provides a simplified view of how I am using the library with MySQL. It uses a single simplified entity to run a query so I can show what is happening. I am using the logging option to output the sql commands generated because this shows where the issue lies. The gist can be found here.

The basic idea is that MySQL uses schema/database interchangeably. And, I want to be able to switch between schemas/dbs in a back-end service that is designed to be multi-tenant. When a user runs the service, the sequelize library should set the appropriate user schema/db. This has the advantage that I never need to change the connection and so default pooling will work to make this a performant solution, I think. I have simplified my gist by removing any code for the user switching and just have a line to comment or uncomment to apply the schema change.

Below is the output from three different scenarios. The first is the default usage w/no schema option being used. The second is using the .schema() method on the entity model to use a different schema. This errors out on the MySQL host with a message that... "Table 'XXX_RootDB.XXX_UserDB.Teams' doesn't exist". However, if I edit the source code to the mysql dialect, I can make this work error free.

I edited the source by adding the property "schemas: true" to the supports object on line 25 in /lib/dialects/mysql/index.js. This seems to fix things and properly prepends the schema name wrapped in backticks in front of the tablename also wrapped in backticks and separated by the default delimiter "."

Logged SQL Output (no schema, XXX_RootDB)

Executing (default): SELECT count(*) AS `count` FROM `Teams` AS `team`;
Executing (default): SELECT `TeamID`, `TeamCode` AS `teamCode`, `TeamName` AS `teamName`, `TeamType` AS `teamType`, `TeamImg` AS `teamImg`, `createdAt`, `updatedAt` 
    FROM `Teams` AS `team`;

Logged SQL Output (schema = XXX_UserDB)

Executing (default): SELECT count(*) AS `count` FROM `FuelStation2_UCLA.Teams` AS `team`;
Executing (default): SELECT `TeamID`, `TeamCode` AS `teamCode`, `TeamName` AS `teamName`, `TeamType` AS `teamType`, `TeamImg` AS `teamImg`, `createdAt`, `updatedAt` 
    FROM `XXX_UserDB.Teams` AS `team`;

Logged SQL Output (mysql dialect updated to support schemas)

Executing (default): SELECT count(*) AS `count` FROM `FuelStation2_UCLA`.`Teams` AS `team`;
Executing (default): SELECT `TeamID`, `TeamCode` AS `teamCode`, `TeamName` AS `teamName`, `TeamType` AS `teamType`, `TeamImg` AS `teamImg`, `createdAt`, `updatedAt` 
    FROM `XXX_UserDB`.`Teams` AS `team`;

At this point, I think this may be more of a bug fix than feature request. What would be involved in setting this up as a pull request for the bug fix? I can clone the repo and issue a pull request. But it looks like there is the need to setup a Docker container and run prebuilt tests for any PR. I have never worked with Docker, so not sure what is involved in all this. Any guidance is appreciated...

[jeffleus]

I am a newbie to PR contribution to open source. So, engaging a project as big as sequelize is a little daunting. Anyway, I am keen to jump in and address my issue. So, I forked the repo, installed Docker Desktop, and ran the mysql test suite using the Docker environment. And, I fail a big chunk of tests when I set the "schemas:true" attribute on the mysql dialect supports object, here.

As I research the unit tests, I see that the `schema.table` approach is written into the unit tests directly. For example, in the create-table.test.js, there are a few examples where the expectation for mysql is in the `schema.table` format. Interestingly, the mariadb dialect expects the format I would like to see for mysql. `foo`.`users`. In each mysql case, there is a supports schemas test, the expected result is looking for a query with `schema.table` in the from clause. So, this appears to be a conscious decision by the developers to implement schemas this way for the mysql dialect.

My guess is that the developers opted for this hack as an approach to make schemas in the mysql dialect work more like schemas in other dialects. Instead of being a true schema, the tables are just named w/a prepended schema. on the name of each table. And so, the tables in each "schema" are all contained in a single database, more like a mssql schema.

This is unfortunate because it prevents me from being able to use the schema approach for multi-tenant design where each tenant resides in its own schema (that is actually treated as a separate database in the mysql mgmt). My motivation for this is better isolation between tenants, as well as easier mgmt of each customers data. I had originally tried changing sequelize between databases, but this approach is muddied significantly by connection pooling and requires disabling pooling and forces new connections to be regularly created, which is a big performance burden.

What is interesting, is that if I simply make the single line change to the mysql dialect by adding "schemas: true", I get the expected behavior in my own client code. Each query uses the desired `schema`.`table` syntax and pulls from each schema/database correctly. The connection to the RDS server never needs to change and my service remains very performant. But when I run this against unit/integ tests, this fails in numerous places because the design decision by sequelize was different.

So, in the end, my change request is would be a breaking change for anyone using the faux-schema approach in mysql. Is there perhaps an option to add an "alternate" behavior to the dialect so that the client can choose the behavior? Perhaps leaving the dialect as schemas:false to default, but allowing the client to override and use the dialect in a "native" schemas approach?

[jeffleus]

New update. So, after running my tests against the Docker container, I started looking closer at the unit and integration tests, in particular those related to schemas. And, it appears that the mariadb dialect works the way I would expect MySQL to work. I guess that shouldn't surprise because of the close lineage of MariaDB and MySQL. My initial attempts to replace the mysql dialect w/the mariadb dialect appear to be successful. I will work to build a set of integration tests this weekend to see if this a viable work-around for me. If this works for me, I will close this feature request.

[papb]

Hi Jeff! Thank you very much for your time, and for posting the detailed responses.

But it looks like there is the need to setup a Docker container and run prebuilt tests for any PR. I have never worked with Docker, so not sure what is involved in all this. Any guidance is appreciated...

One of my next goals in Sequelize is to clarify how to setup these kind of things. Adding clearer instructions, especially for first-time contributors. Looks like you managed to make it work, so I will leave it at this for now, but be sure that in the close future the guides for contributing will be better!

I am a newbie to PR contribution to open source. So, engaging a project as big as sequelize is a little daunting. Anyway, I am keen to jump in and address my issue. So, I forked the repo, installed Docker Desktop, and ran the mysql test suite using the Docker environment.

Thanks again for being willing to help!! Awesome!!

My guess is that the developers opted for this hack as an approach to make schemas in the mysql dialect work more like schemas in other dialects. Instead of being a true schema, the tables are just named w/a prepended schema. on the name of each table.

This looks like a valid guess, but I have another guess that should be considered as well: in the past, Sequelize did not support MariaDB yet, only MySQL. Perhaps when MariaDB support was added, this behavior was "fixed" for it but MySQL was left unchanged ("wrong"). I would try investigating the git history to see if my guess is correct, but I barely have time to reply to you here... If you could check it out, it would be nice.

So, in the end, my change request is would be a breaking change for anyone using the faux-schema approach in mysql. Is there perhaps an option to add an "alternate" behavior to the dialect so that the client can choose the behavior? Perhaps leaving the dialect as schemas:false to default, but allowing the client to override and use the dialect in a "native" schemas approach?

I think you made a great analysis here. I am super in favor of creating a new feature as an option to choose between "real schemas" and "fake schemas". This way nothing would break and users like you would be happy!

I will work to build a set of integration tests this weekend to see if this a viable work-around for me. If this works for me, I will close this feature request.

If it works for you, please let me know (and every other possible reader!) here, it will be considered a workaround, but please don't close the issue, I think the idea is very valid.

[papb]

Note: I may have missed something because I am in a bit of a rush right now, feel free to remind me of anything and/or proceed the discussion :)

[jeffleus]

A little food poisoning wiped me out last week. I will pickup this work again in the next couple of days. But I did get a chance to look back through the commit history on the mariadb dialect. There is an explicit comment by @rusher that he noted the implementation for schemas in MySQL is probably incorrect. But he did not modify this because it would be a breaking change. However, since he was releasing the mariadb, he chose to correctly implement schemas. @rusher could you perhaps comment on the use of the mariadb dialect when working w/a mysql db? my plan is to swap the dialect in my specific project and test things out. I will report back here after...

note, comment can be found in PR#10192 here

[marcogrcr]

For people experiencing issues in AWS Lambda, I have created a pull request that documents how to properly configure sequelize for Lambda. Hopefully you'll be able to see it in https://sequelize.org/master/manual/aws-lambda.html once it gets merged. In the meantime, please check #12642 out and provide feedback if you find any inaccuracies or space for improvement.

[charlie-s]

Did this issue get resolved or go stale? I am available to help implement immediately.

[github-actions]

This issue has been automatically marked as stale because it has been open for 7 days without activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

[ephys]

This was implemented in #14999