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

Schema prefix should be ignored only for SQLite, MySQL driver (among others) is broken #5447

Closed
marrasen opened this issue Jan 31, 2020 · 10 comments

Comments

@marrasen
Copy link

Someone filed an issue that connections using SQLite didn't ignore schema: #4598

A fix was made and merged: #4599

The fix ignores schema for ALL drivers except Postgres and SqlServer, see the changes in this commit: d8f1c81

This commit adds SapHana to the list of driverrs that supports schemas: ec90341#diff-42c71650de3df6bb209faec11d1ff51b

Issue #4820 reports that schemas are ignored for Oracle, to fix this we need to add Oracle driver as well.

MySQL driver is broken and would also need to be added to this if statement. We can't upgrade TypeOrm due to this bug.

In my opinion, the original fix should have ignored schema ONLY for SQLite, the other way around.

This line:

if (this.schema && ((this.connection.driver instanceof PostgresDriver) || (this.connection.driver instanceof SqlServerDriver) || (this.connection.driver instanceof SapDriver))) {

@imnotjames
Copy link
Contributor

According to the MySQL documentation

CREATE DATABASE creates a database with the given name. To use this statement, you need the CREATE privilege for the database. CREATE SCHEMA is a synonym for CREATE DATABASE as of MySQL 5.0.2.

As far as I can tell, MySQL does not have the concept of a schema - it's just a synonym for database. Is that correct?

@marrasen
Copy link
Author

marrasen commented Jul 7, 2021

Yes, schema is just a synonym for database.

But in MySQL a "database" actually is a "schema". It's not a separate "database" or anything, just another schema on the same server. So the name "database" is a little misleading imo. "SELECT * FROM inventory.item" selects from the database named "inventory". I think that is why they added the synonym.

@imnotjames
Copy link
Contributor

In MySQL a database is a database because that's the term they chose. We follow their terminology in this case which is why for MySQL it's implemented to support setting database but not schema>

How is this broken if the schema is ignored & database activates the mysql database?

If I add mysql to that if statement we get cases where you can specify Both a schema and a database which is not correct. Then this would for sure be broken.

What's the use case you have for this?

@marrasen
Copy link
Author

marrasen commented Jul 7, 2021

In MySQL there is no difference if you use the term "schema" or if you use the term "database". And what if you have a typeorm project that you want to use for the SQLite engine in one instance and for MySQL in another, the code will not be portable between the two engines because of this, and if the reason for that is because MySQL chose the word "database" 26 years ago than I think it's unfortunate. If different engines use different words for the same thing, isn't typeorm supposed to help the developers bridge such gaps and make the code portable? Or am I missing something?

I generated my table definitions using typeorm-model-generator, it used the term Schema and it all worked fine until after a minor update when it suddenly stopped working due to this change. It took several days of trouble shooting before realizing that this was the problem.

Edit: I realize that maybe this comment sounds a bit rude. I don't mean to be. I'm sorry. I think TypeOrm is a great project and I really appreciate all the work you put into it. Thank you for that

@imnotjames
Copy link
Contributor

It didn't come off as rude at all - not to worry.

Are you suggesting to effectively change all instances of database to schema for MySQL? Wouldn't that break existing users' code?

Or do you mean to use schema as if it were the database? What happens when both the schema and database are provided?

@marrasen
Copy link
Author

marrasen commented Jul 7, 2021

Good question.

The problem for me was that the fix I mentioned in the original post broke all our MySQL code. Using schema was fine and then suddenly it wasn't. The query would be made by just ignoring the schema and would use the default specified by the connection, which made some entities work and others didn't. It was hard for us to trouble shoot.

The fix causing this wasn't "using schema should not be possible on MySQL driver", it was for ignoring schema for SQLite, but suddenly ignored schema for everything except Postgres and SqlServer. The fix broke the code for others as well using other drivers.

When I read the code for buildTablePath it seems like schema and database are treated the same for all SQL drivers except for SqlServerDriver and PostgresDriver. In Postgres you are required to use schema, for everyone else you have to use database. Maybe this is a problem for code portability.

We have changed our code to use database now, so we are fine and we know about this. But since MySQL calls it both "schema" and "database" these days, having "schema" being just simply ignored may cause others the same confusion. Reading the documentation doesn't describe the distinction between the two either. Maybe the fix is simply to explain it in the documentation :)

@imnotjames
Copy link
Contributor

I've refactored the way a lot of buildTablePath and schema selection works.

It's now almost entirely part of the driver instead of within the `EntityMetadata. With that in mind, should anyone want to try to make it work where it's used interchangably that can be done. I'm not planning on that. I've also updated other databases to support schemas - not just SQL Server & Postgres - when they should be able to.

As it stands, while the concept of a schema vs a database is the same in MySQL land it's different everywhere else. As such, I don't think we should conflate the two just because MySQL chose to make them synonyms for some reason.

@imnotjames
Copy link
Contributor

Related PR: #7856

@spamshaker
Copy link

what about sqlite attach? Is it supported?

@spamshaker
Copy link

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

No branches or pull requests

3 participants