google/keyczar

Patch: Use UTF-8

GoogleCodeExporter opened this issue · 6 comments

There are a number of places I found in the code where the default platform 
encoding is used.  That's not guaranteed to be UTF-8, or even the same across 
platforms.  We should specify the encoding for strings, and UTF-8 is already 
defined in KeyCzar.java as DEFAULT_ENCODING.

https://github.com/justinsb/keyczar/commit/80a1865b8c578f973fd0927d1ed23bbaace48
694.diff

Original issue reported on code.google.com by jus...@fathomdb.com on 12 Aug 2013 at 10:22

Is there a reason DEFAULT_ENCODING isn't used in this fix?

Original comment by jtu...@gmail.com on 13 Aug 2013 at 4:56

  • Added labels: Type-Patch, Implementation-Java
  • Removed labels: Type-Defect
Well, to be honest, I didn't find DEFAULT_ENCODING until later.

But actually, when we use a Charset, we don't need to handle 
UnsupportedEncodingException, because we already have the Charset.  Makes the 
code cleaner, might even be a little faster.

There's definitely a case that Utils.UTF8 should be renamed 
Utils.DEFAULT_ENCODING, and maybe that DEFAULT_ENCODING should then be 
repointed to consistently use the Charset from Utils.  I would suggest that 
UTF8 is a better name than DEFAULT_ENCODING, because DEFAULT_ENCODING suggests 
to me the platform default encoding, and sets off red flags in my mind.

But I wanted to start with a sensible sized patch, rather than refactor 
everything :-)

Original comment by jus...@fathomdb.com on 13 Aug 2013 at 5:22

It's a good case for Util.UTF_8 for everything. Shawn Wilden's the maintainer 
and on the keyczar-discuss@googlegroups.com he said it was good if you wanted 
to change all DEFAULT_ENCODINGs to Util.UTF_8 (and you should join the mailing 
list if you haven't already).


Original comment by jtu...@gmail.com on 13 Aug 2013 at 6:16

  • Changed state: Accepted
Here's the latest diff, with DEFAULT_ENCODING replaced by Util.UTF_8.
https://github.com/justinsb/keyczar/compare/upstream...utf8.diff

Original comment by jus...@fathomdb.com on 13 Aug 2013 at 8:37

We are hitting this issue in our project. Any information as to when will a new 
version be released incorporating the patch?

Original comment by rafael.f...@nubank.com.br on 22 Jan 2014 at 4:50

Thanks for the reminder. I'll pull this in and get a new release out this week.

Original comment by swillden@google.com on 22 Jan 2014 at 4:57