Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Improve JDBC Url parsing #366

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
# for free to join this conversation on GitHub. Already have an account? # 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