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

makes finagle-mysql charsets extensible #150

Closed
wants to merge 3 commits into from
Closed

makes finagle-mysql charsets extensible #150

wants to merge 3 commits into from

Conversation

mosesn
Copy link
Contributor

@mosesn mosesn commented Apr 2, 2013

motivation

Some people have mysql databases with different character encodings. finagle-mysql doesn't support a lot right now, but it should in the future.

implementation

This is a more complicated problem than you might think at first, because different columns can theoretically be different charsets within a single database.

todo

prepared statements

I only added support for Latin-1 in this PR, because it lets me punt on encoding prepared statements. For prepared statements, we don't need to declare anything about the charset to mysql, but we do need to encode them. Because latin-1 is compatible with utf-8, this just assumes every string in a prepared statement will use utf-8. More work will need to be done to guarantee that every string in a prepared statement is encoded properly.

other charsets

The other part of this that isn't complete is that the conversion from mysql collation number to a java.nio.charset is completely punted on, and it always returns utf-8. In the future, there should be a map from collations to charsets.

thanks

Thanks @roanta for bearing with me as I muddled through the finagle-mysql code, and providing guidance.

@mosesn
Copy link
Contributor Author

mosesn commented Apr 8, 2013

@roanta tells me that this has been merged internally. I think that only up until a51ec07 was merged internally. I pushed to this branch, forgetting that it hadn't been pushed back into the open source repo.

I'm not sure what to do in this situation. Should I revert a2df331 on this branch? I have a new branch with a new pull request in #154.

@roanta
Copy link
Contributor

roanta commented Apr 12, 2013

I've pulled this in along with #154. Should sync into the public repo shortly. Thanks!

@roanta roanta closed this Apr 12, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants