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

MySQL - Table meta data fix #1924

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
3 participants
@Asamsig
Copy link
Contributor

Asamsig commented Jun 12, 2018

Fixes issue #1692 for MySQL.

Tests in JdbcMetaTest cover this change, since they call the method defaultTables.
This makes PR #1717 somewhat obsolete, at least in regards to MySQL.

MySQL connector/J 8.x has changed nullCatalogMeansCurrent to now default to false, so the MySQLProfile defaultTables now fetches the current catalog from the connection.

We currently only run the test suite on MySQL 5.x series, but I've tested locally with 6.x and 8.x as well.

I have not looked into #1485 yet, but I can see that the SQLServerProfile has its own override of defaultTables. I'll look into this separately.

@hvesalai hvesalai added this to the 3.3 milestone Jun 13, 2018

@hvesalai

This comment has been minimized.

Copy link
Member

hvesalai commented Jun 13, 2018

Could you include a similar fix also for other DBs. I don't like genreal problems being fixed for only one DB at a time. At least Oracle is reported to also have the same problem.

@hvesalai

This comment has been minimized.

Copy link
Member

hvesalai commented Jun 13, 2018

The tests are failing because of an other issue. Can you rebase to master

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Jun 13, 2018

I'll have a look at SQLServer and Oracle, is there an issue for Oracle, so I can see what steps I need to reproduce?

I'll look into it, hopefully over the course of the week.

@hvesalai

This comment has been minimized.

Copy link
Member

hvesalai commented Jun 13, 2018

I think #1692 mentions Oracle

@hvesalai

This comment has been minimized.

Copy link
Member

hvesalai commented Jun 13, 2018

And #1717 fixes that. The only thing missing from #1717 is a test to proove that it does not break anything or somebody to say that the test is already there and no further tests are not needed.

MySQL driver 6.x and later doesn't support null in name pattern or sc…
…heme pattern, the correct way in later versions is passing '%'. MySQL driver 8.x now defaults nullCatalogMeansCurrent to false, therefore the current catalog has to be passed in.

Added "%" as tableNamePattern on SQLServer, Postgres, Oracle. Oracle has started not accepting null in newer versions, and "%" should be semantically the same.

@Asamsig Asamsig force-pushed the Asamsig:issue/1692-name-pattern-mysql branch from 678171c to 27ed3f5 Jun 16, 2018

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Jun 16, 2018

I've rebased to master.

Also I've investigated other profiles for the same kind of issue, and it seems that Oracle does indeed suffer under this on newer versions. Progres supports "%" with the same behaivior as null, so passing "%" seems more future safe, and this is how JDBC documents the prefered way.

The changes are covered by tests in JdbcMetaTest.

@hvesalai

This comment has been minimized.

Copy link
Member

hvesalai commented Aug 29, 2018

Should this now be merged or is there something more? What about the Oracle aspect?

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Aug 29, 2018

I can see it wasn't clear from my earlier comment, but yes, it should be ready to be merged.

@TimMoore

This comment has been minimized.

Copy link

TimMoore commented Aug 30, 2018

Thanks, @Asamsig! The issue that blocked #1717 is that the tests in JdbcMetaTest obviously don't cover this, or else they wouldn't have been passing in the first place.

@Asamsig

This comment has been minimized.

Copy link
Contributor Author

Asamsig commented Aug 30, 2018

Actually the changes are covered. The problem is that the tests are only run with one version of the database drivers, and newer versions are behaving differently, so unless we add more database drivers to our CI test suite, we won't be able to test it.

What I've been is manually changed my database driver locally and repeatedly run the tests. To verify that the change works.

@hvesalai hvesalai merged commit 762c8e3 into slick:master Jan 29, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment