Strip newlines from base 64 encoder. #3

Merged
merged 2 commits into from May 6, 2011

Conversation

5 participants
@kmonkeyjam
Contributor

kmonkeyjam commented Apr 6, 2011

We used to pass in a don't use newline flag when we used the apache commons codec base 64 encoder. There is not such a flag in the sun.misc encoder. If you override the bytesPerLine method to return Int.MaxInt, it will give you a heap overflow. So we just explicitly strip them.

@stevej

This comment has been minimized.

Show comment
Hide comment
@stevej

stevej Apr 6, 2011

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

stevej commented Apr 6, 2011

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

@kmonkeyjam

This comment has been minimized.

Show comment
Hide comment
@kmonkeyjam

kmonkeyjam Apr 6, 2011

Contributor

Wait, description not title. Adding now.

On Wed, Apr 6, 2011 at 1:04 PM, Tina Huang tina@monkey.name wrote:

I added one, but don't know how to re-send the pull request.  :-(

On Wed, Apr 6, 2011 at 1:04 PM, stevej
reply@reply.github.com
wrote:

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

Reply to this email directly or view it on GitHub:
#3 (comment)

Contributor

kmonkeyjam commented Apr 6, 2011

Wait, description not title. Adding now.

On Wed, Apr 6, 2011 at 1:04 PM, Tina Huang tina@monkey.name wrote:

I added one, but don't know how to re-send the pull request.  :-(

On Wed, Apr 6, 2011 at 1:04 PM, stevej
reply@reply.github.com
wrote:

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

Reply to this email directly or view it on GitHub:
#3 (comment)

@kmonkeyjam

This comment has been minimized.

Show comment
Hide comment
@kmonkeyjam

kmonkeyjam Apr 6, 2011

Contributor

I added one, but don't know how to re-send the pull request. :-(

On Wed, Apr 6, 2011 at 1:04 PM, stevej
reply@reply.github.com
wrote:

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

Reply to this email directly or view it on GitHub:
#3 (comment)

Contributor

kmonkeyjam commented Apr 6, 2011

I added one, but don't know how to re-send the pull request. :-(

On Wed, Apr 6, 2011 at 1:04 PM, stevej
reply@reply.github.com
wrote:

needs a description.

On Wed, Apr 6, 2011 at 12:56 PM, kmonkeyjam
reply@reply.github.com
wrote:

Reply to this email directly or view it on GitHub:
#3

Reply to this email directly or view it on GitHub:
#3 (comment)

@9len

This comment has been minimized.

Show comment
Hide comment
@9len

9len Apr 6, 2011

can we add a test to make sure that the decoder doesn't care about newlines one way or the other?

9len commented Apr 6, 2011

can we add a test to make sure that the decoder doesn't care about newlines one way or the other?

@kmonkeyjam

This comment has been minimized.

Show comment
Hide comment
@kmonkeyjam

kmonkeyjam Apr 6, 2011

Contributor

Added a test.

Contributor

kmonkeyjam commented Apr 6, 2011

Added a test.

@9len

This comment has been minimized.

Show comment
Hide comment
@9len

9len Apr 6, 2011

birdcage/util is now the canonical home of util code changes, the change should be made there...

9len commented Apr 6, 2011

birdcage/util is now the canonical home of util code changes, the change should be made there...

@kmonkeyjam

This comment has been minimized.

Show comment
Hide comment
@kmonkeyjam

kmonkeyjam Apr 6, 2011

Contributor

I search in the twitter github and get nothing for birdcage.

On Wed, Apr 6, 2011 at 1:44 PM, 9len
reply@reply.github.com
wrote:

birdcage/util is now the canonical home of util code changes, the change should be made there...

Reply to this email directly or view it on GitHub:
#3 (comment)

Contributor

kmonkeyjam commented Apr 6, 2011

I search in the twitter github and get nothing for birdcage.

On Wed, Apr 6, 2011 at 1:44 PM, 9len
reply@reply.github.com
wrote:

birdcage/util is now the canonical home of util code changes, the change should be made there...

Reply to this email directly or view it on GitHub:
#3 (comment)

@9len

This comment has been minimized.

Show comment
Hide comment
@9len

9len Apr 6, 2011

birdcage is an internal repo.

9len commented Apr 6, 2011

birdcage is an internal repo.

@robey

This comment has been minimized.

Show comment
Hide comment
@robey

robey May 6, 2011

i'll just merge it here.

robey commented May 6, 2011

i'll just merge it here.

robey pushed a commit that referenced this pull request May 6, 2011

Robey Pointer
Merge pull request #3 from kmonkeyjam/master
Strip newlines from base 64 encoder.

@robey robey merged commit 2275ced into twitter:master May 6, 2011

@som-snytt

This comment has been minimized.

Show comment
Hide comment
@som-snytt

som-snytt Aug 14, 2011

This change was zapped when the code was moved to util-codec, but the correct change would have been to override BASE64Encoder.encodeLineSuffix and not emit the newline. This change, to replace \n instead of util.Properties.lineSeparator, breaks on Windows (where some of us are dual-booted); or at least the test breaks, since the decoder ignores [\r\n]*. But this behavior is arguably wrong, in so far as CR-LF is called for, and is supplied by Commons Codec which is used now:
http://tools.ietf.org/html/rfc2045#section-2.1

This change was zapped when the code was moved to util-codec, but the correct change would have been to override BASE64Encoder.encodeLineSuffix and not emit the newline. This change, to replace \n instead of util.Properties.lineSeparator, breaks on Windows (where some of us are dual-booted); or at least the test breaks, since the decoder ignores [\r\n]*. But this behavior is arguably wrong, in so far as CR-LF is called for, and is supplied by Commons Codec which is used now:
http://tools.ietf.org/html/rfc2045#section-2.1

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