Add a percent-encoding converter.

Add a percent-encoding converter.

This is useful for percent-encoding binary or streaming data.

R=lrn@google.com

Committed: https://github.com/dart-lang/convert/commit/86b65bdf07ba3bf9bd7973682c503c819579c3f6

[nweiz, 2015-10-07 22:23:24]

nweiz@google.com changed reviewers:
+ lrn@google.com

[nweiz, 2015-10-07 22:23:24]

I did a pretty naïve implementation of this under the assumption that you'd be
better at figuring out how to improve it than I would.

[Lasse Reichstein Nielsen, 2015-10-08 05:44:32]

When is percent-encoding used for large amounts of data? I have only seen it
used in URIs and HTTP messages (application/x-www-form-urlencoded), but in those
cases you usually have the entire value available and can just use
Uri.encodeComponent.

[Lasse Reichstein Nielsen, 2015-10-08 10:30:37]

LGTM if you think we need this codec.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent.dart
File lib/src/percent.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent.dart#newcode19
lib/src/percent.dart:19: // character.
That's not plain percent-encoding. It's www-form-urlencoding which is its own
encoding. Percent-encoding per RFC 3986 (2.1 Percent-Encoding) doesn't do that.
I'd probably make a separate codec called wwwFormEncoding.

RFC 3986 does say that it encodes characters that have meaning in the context,
so you may want to either specify a context or allow the user to specify it.
A context is basically a set of allowed ASCII characters.
Defaulting to "query component" is probably a good start, that's where you will
need it.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dart
File lib/src/percent/decoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:15: const percentDecoder = const
PercentDecoder._();
Consider typing the constant.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:19: /// A converter that decodes percentadecimal
strings into byte arrays.
percentadecimal? I'm guessing "percent encoded".
Replace everywhere it occurs (most other places just with "percent").

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:100: throw new FormatException("Input ended with
incomplete encoded byte.");
Consider having a version of this that is passed the input and index.

So:
 void close() { _close(null, null); }
 void _close(input, index) {
   if (_lastDigit != null) { 
     throw new FormatException("...", input. index);
   }
   _sink.close();
 }

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:107: /// A conversion sink for chunked
percentadecimal decoding from UTF-8 bytes.
percentadecimal :)

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:175: if (codeUnit > 0xf7) {
This should also be checking for codeUnit < 0.

For optimizing, I'd just collect the bitwise-or of all the codeUnits and then
check afterwards if any of them were bad. Heck, even wait with adding the code
units to the buffer, just loop searching for the first '%', and don't do
anything until it's found. If there are none, you can just add the entire input
to the buffer.

var codeUnitOr = 0;
int sliceStart = start;
for (var i = start; i < end; i++) {
  var codeUnit = codeUnits[i];
  if (codeUnit != $percent) {
    codeUnitOr |= codeUnit;
    continue;
  }
  if (codeUnitOr < 0 || codeUnitOr > 127) {
    // find error and throw.
  }
  if (i > sliceStart) {
    buffer.addAll(codeUnits.getRange(sliceStart, i));
  }
  // handle percent like now. 
  
  sliceStart = i + 1;
}

if (codeUnitOr < 0 || codeUnitOr > 127) {
  // find error and throw.
}
if (end > sliceStart) {
  if (start == sliceStart) {
    buffer.addAll(codeUnits);
  } else {
    buffer.addAll(codeUnits.getRange(sliceStart, end));
  }
}

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dart
File lib/src/percent/encoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:31: /// A conversion sink for chunked
percentadecimal encoding.
Ah, that's where percentadecimal comes from :)

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:65: (byte >= $A && byte <= $Z) ||
You can save a test by doing case-canonicalization before testing.:

var ucLetter = byte | 0x20;
if ((ucLetter >= $a && ucLetter <= %z) ||
    byte == $dash || ....

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:90: throw new FormatException("Invalid byte
0x${byte.toRadixString(16)}.",
This will look bad if "byte" is negative.
Maybe: ${byte < 0 ? "-" : ""}0x${byte.abs().toRadixString(16)}
Ditto for the hex encoder.

https://codereview.chromium.org/1393003003/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/utils.dart#newcode9
lib/src/utils.dart:9: /// Returns the digit (0 through 15) corresponding to the
percentadecimal code unit
Long lines - due to percentadecimal.

[nweiz, 2015-10-08 20:41:33]

Code review changes

[nweiz, 2015-10-08 20:44:52]

> When is percent-encoding used for large amounts of data? I
> have only seen it used in URIs and HTTP messages 
> application/x-www-form-urlencoded), but in those cases you
> usually have the entire value available and can just use
> Uri.encodeComponent.

It's less about large/streaming data and more about encoding binary data, as
well as (eventually) having a place to but more fine-grained configuration about
which characters are encoded and which aren't.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dart
File lib/src/percent/decoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:15: const percentDecoder = const
PercentDecoder._();
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> Consider typing the constant.

I really don't like doing this for final/const fields where the RHS is exactly
the correct type already.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:19: /// A converter that decodes percentadecimal
strings into byte arrays.
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> percentadecimal? I'm guessing "percent encoded".
> Replace everywhere it occurs (most other places just with "percent").

I started this file as a search-replace of the hex decoder, but I thought I
caught all these :p.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:100: throw new FormatException("Input ended with
incomplete encoded byte.");
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> Consider having a version of this that is passed the input and index.
> 
> So:
>  void close() { _close(null, null); }
>  void _close(input, index) {
>    if (_lastDigit != null) { 
>      throw new FormatException("...", input. index);
>    }
>    _sink.close();
>  }

Done. Made the same change in the hex decoder as well.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:107: /// A conversion sink for chunked
percentadecimal decoding from UTF-8 bytes.
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> percentadecimal :)

Done.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:175: if (codeUnit > 0xf7) {
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> This should also be checking for codeUnit < 0.
> 
> For optimizing, I'd just collect the bitwise-or of all the codeUnits and then
> check afterwards if any of them were bad. Heck, even wait with adding the code
> units to the buffer, just loop searching for the first '%', and don't do
> anything until it's found. If there are none, you can just add the entire
input
> to the buffer.
> 
> var codeUnitOr = 0;
> int sliceStart = start;
> for (var i = start; i < end; i++) {
>   var codeUnit = codeUnits[i];
>   if (codeUnit != $percent) {
>     codeUnitOr |= codeUnit;
>     continue;
>   }
>   if (codeUnitOr < 0 || codeUnitOr > 127) {
>     // find error and throw.
>   }
>   if (i > sliceStart) {
>     buffer.addAll(codeUnits.getRange(sliceStart, i));
>   }
>   // handle percent like now. 
>   
>   sliceStart = i + 1;
> }
> 
> if (codeUnitOr < 0 || codeUnitOr > 127) {
>   // find error and throw.
> }
> if (end > sliceStart) {
>   if (start == sliceStart) {
>     buffer.addAll(codeUnits);
>   } else {
>     buffer.addAll(codeUnits.getRange(sliceStart, end));
>   }
> }

Done.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dart
File lib/src/percent/encoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:31: /// A conversion sink for chunked
percentadecimal encoding.
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> Ah, that's where percentadecimal comes from :)

Done.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:65: (byte >= $A && byte <= $Z) ||
On 2015/10/08 10:30:36, Lasse Reichstein Nielsen wrote:
> You can save a test by doing case-canonicalization before testing.:
> 
> var ucLetter = byte | 0x20;
> if ((ucLetter >= $a && ucLetter <= %z) ||
>     byte == $dash || ....

Done.

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/encoder.dar...
lib/src/percent/encoder.dart:90: throw new FormatException("Invalid byte
0x${byte.toRadixString(16)}.",
On 2015/10/08 10:30:37, Lasse Reichstein Nielsen wrote:
> This will look bad if "byte" is negative.
> Maybe: ${byte < 0 ? "-" : ""}0x${byte.abs().toRadixString(16)}
> Ditto for the hex encoder.

Done.

https://codereview.chromium.org/1393003003/diff/1/lib/src/utils.dart
File lib/src/utils.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/utils.dart#newcode9
lib/src/utils.dart:9: /// Returns the digit (0 through 15) corresponding to the
percentadecimal code unit
On 2015/10/08 10:30:37, Lasse Reichstein Nielsen wrote:
> Long lines - due to percentadecimal.

Done.

[nweiz, 2015-10-08 20:45:07]

Committed patchset #2 (id:20001) manually as
86b65bdf07ba3bf9bd7973682c503c819579c3f6 (presubmit successful).

[Lasse Reichstein Nielsen, 2015-10-09 09:07:13]

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dart
File lib/src/percent/decoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:15: const percentDecoder = const
PercentDecoder._();
Will the dartdoc show the correct type for the constant if you don't?

[nweiz, 2015-10-09 19:52:55]

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dart
File lib/src/percent/decoder.dart (right):

https://codereview.chromium.org/1393003003/diff/1/lib/src/percent/decoder.dar...
lib/src/percent/decoder.dart:15: const percentDecoder = const
PercentDecoder._();
On 2015/10/09 09:07:13, Lasse Reichstein Nielsen wrote:
> Will the dartdoc show the correct type for the constant if you don't?

It should; if not, that's an issue with dartdoc.

In this case in particular, this isn't exposed publicly, so that's a non-issue.