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

HikariCP: config property for init connection sql added #1293

Merged
merged 2 commits into from
Feb 23, 2019

Conversation

sbeigel
Copy link
Contributor

@sbeigel sbeigel commented Feb 6, 2019

Hikari supports setConnectionInitSql() to register custom SQL to initialize a new connection. This is handy to configure the character set for the connection (e.g. "SET NAMES utf8mb4" in MySQL to make emojis etc. work ;)
I added a play config option to setup this property.

@asolntsev asolntsev self-requested a review February 6, 2019 20:20
Copy link
Contributor

@asolntsev asolntsev left a comment

Choose a reason for hiding this comment

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

@sbeigel Could you please add description of the new parameter to documentation/manual/configuration.textile? I know @xael-fry will definitely ask for it. :)

@tazmaniax
Copy link
Collaborator

@sbeigel just looking at the Connector/J 5.1 documentation it explicitly warns...

Do not issue the query SET NAMES with Connector/J, as the driver will not detect that the character set has been changed by the query, and will continue to use the character set configured when the connection was first set up.

and even recommends by default not to specify the connection string parameter characterEncoding=UTF-8 and just rely on the server setting...

The driver automatically uses the encoding specified by the server. For example, to use the 4-byte UTF-8 character set with Connector/J, configure the MySQL server with character_set_server=utf8mb4, and leave characterEncoding and connectionCollation out of the Connector/J connection string. Connector/J will then autodetect the UTF-8 setting.

but curious if you have a different experience

@tazmaniax
Copy link
Collaborator

Related to this I've raised an issue #1294 and pull request #1295 to remove the current character encoding and collation defaults hardcoded by Play which are not needed and also sets the collation to "utf8_general_ci" which might not be desired especially if "utf8mb4" is configured on the server.

@sbeigel
Copy link
Contributor Author

sbeigel commented Feb 11, 2019

but curious if you have a different experience

Sometimes you can't (easily) change the server settings, so I was looking for another solution. And executing "SET NAMES" for every new connection seems to work, I can read and write extended unicode chars (emojis) without problems.

But this is only one (my current) usecase for this PR, so even if it's not the recommended or even a good solution for the utf8mb4 problem, having this setting in the play config is still useful!

@tazmaniax
Copy link
Collaborator

tazmaniax commented Feb 11, 2019

Sometimes you can't (easily) change the server settings, so I was looking for another solution. And executing "SET NAMES" for every new connection seems to work, I can read and write extended unicode chars (emojis) without problems.

Right I see your point particularly if you're using Connector/J 5.1.46 (and earlier) without character_set_server=utf8mb4, then because characterEncoding=UTF-8 maps to utf8 which is really utf8mb3 then quite possibly executing "SET NAMES utf8mb4" is the only option. However I would still be concerned about sending utf8mb4 encoded characters to a database that hadn't necessarily been configured to store utf8mb4 characters in the first place.

But this is only one (my current) usecase for this PR, so even if it's not the recommended or even a good solution for the utf8mb4 problem, having this setting in the play config is still useful!

Definitely agree and I'm sure it will come in handy for other things, thx!

@xael-fry xael-fry merged commit 6881747 into playframework:master Feb 23, 2019
@xael-fry
Copy link
Member

Merged in master
thanks

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

Successfully merging this pull request may close these issues.

4 participants