Skip to content

Improve JDBC Url parsing #366

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

Merged
merged 5 commits into from
Jan 9, 2023
Merged

Conversation

jtjeferreira
Copy link
Contributor

fixes #365

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for promptly updating the PR! This PR already looks great to me but I'd like to wait for others' feedback for a few more days. After that, we can release a new version including this change.

@@ -26,35 +25,39 @@ private[scalikejdbc] trait JasyncConfiguration {

val defaultConfiguration = new Configuration("")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this field is no longer used, but I didnt remove it due to backwards compatibility. If that is not an issue I can remove it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, can you add @deprecated annotation to the public field? The since version will be 0.16.0

@jtjeferreira
Copy link
Contributor Author

Thanks a lot for promptly updating the PR! This PR already looks great to me but I'd like to wait for others' feedback for a few more days. After that, we can release a new version including this change.

ok I can wait since I have an workaround. Nevertheless I used publishLocal and tested the changes in my project successfully.

Another question: should we fix scalikejdbc.JDBCUrl.apply? Or deprecate it so we can remove it in the future?

@jtjeferreira
Copy link
Contributor Author

Also do you think the code here can now be simplified?

url match {
case _ if url.startsWith("jdbc:postgresql://") =>
new PostgreSQLConnectionPoolImpl(url, user, password, settings)
case _ if url.startsWith("jdbc:mysql://") =>
new MySQLConnectionPoolImpl(url, user, password, settings)
case HerokuPostgresRegexp(_user, _password, _host, _dbname) =>
// Heroku PostgreSQL
val _url = "jdbc:postgresql://%s/%s".format(_host, _dbname)
new PostgreSQLConnectionPoolImpl(_url, _user, _password, settings)
case HerokuMySQLRegexp(_user, _password, _host, _dbname) =>
// Heroku MySQL
// issue #5 Error: database name is too long
// val defaultProperties = """?useUnicode=yes&characterEncoding=UTF-8&connectionCollation=utf8_general_ci"""
val defaultProperties = ""
val addDefaultPropertiesIfNeeded = MysqlCustomProperties
.findFirstMatchIn(url)
.map(_ => "")
.getOrElse(defaultProperties)
val _url = "jdbc:mysql://%s/%s".format(
_host,
_dbname + addDefaultPropertiesIfNeeded
)
new MySQLConnectionPoolImpl(_url, _user, _password, settings)

@seratch
Copy link
Member

seratch commented Dec 26, 2022

Another question: should we fix scalikejdbc.JDBCUrl.apply? Or deprecate it so we can remove it in the future?

It can still be useful for some simple use cases, so I don't think it should be deprecated immediately. But I agree that the implementation should be complete in the future. I will create a new issue on the project side.

UPDATE: Here is the issue scalikejdbc/scalikejdbc#1825

@seratch
Copy link
Member

seratch commented Dec 26, 2022

Also do you think the code here can now be simplified?

No rush but simplifying the code is great!

@seratch seratch merged commit ff796c4 into scalikejdbc:master Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

Database does not exist error due to bad JDBCUrl parsing
2 participants