Create a package that implements a JSON-RPC 2.0 server.

Create a package that implements a JSON-RPC 2.0 server.

R=rnystrom@google.com
BUG=17492
Committed: https://code.google.com/p/dart/source/detail?r=34223

[Bob Nystrom, 2014-03-20 18:25:58]

I really really like this. I want to run an API idea past you. I really like the
Parameters object and the validation it does, but I think the methods are a
little strange. What do you think about something like:

Instead of getNamed("message"):

  return params["message"].value

Instead of getNum("minuend"):

  return params["minuend"].asNum

Instead of getBool("descending", orElse: ...):

  params["descending"].asBoolOrElse(...)

Instead of getPositional(123):

  params[123].value

In other words, use a subscript operator which takes strings (on maps) and ints
(on lists) to access structured properties. That in turn always returns another
wrapped params object. Then the validation is done using an "as___" method to
unwrap to the primitive value.

I think this makes for a more readable API, especially with nested stuff:

  params["assets"][0]["path"].asString

It also supports nested lists. The downsides I can think of are:

1. If you don't care to validate, you have to remember to do .value to unwrap
the Parameter object.

2. The overhead of wrapping.

What do you think?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md
File pkg/json_rpc_2/README.md (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md#new...
pkg/json_rpc_2/README.md:64: `Server.parseRequest`:
How about "handleRequest" since this is the common case?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md#new...
pkg/json_rpc_2/README.md:79: you can also call `Server.handleRequest` directly:
And then maybe make this "handleMessage" or "handleParsedRequest"?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/error_cod...
File pkg/json_rpc_2/lib/error_code.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/error_cod...
pkg/json_rpc_2/lib/error_code.dart:5: /// Error codes defined in the JSON-RPC
2.0 specificiation.
Hyperlink?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
File pkg/json_rpc_2/lib/src/parameters.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:21: /// The underlying value of the
parameters.
"parameters" -> "parameters object"

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:25: /// parameters map.
How about a bit explaining how this is formatted? I assume it's like:

someArray.123.someMap.key

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:77: _getTyped(name, 'an integer', (value)
=> value is int, orElse);
This will do weird things in dart2js. If the value is 1.0, it will fail in the
VM but succeed in dart2js.

I guess it doesn't matter for now since this is dart:io-only.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:109: /// It's safe to modify the returned
list.
If we claim to support this, we're stuck with the performance hit of copying
forever. That could be bad if someone uses this for something perf-heavy like
isolate communication.

How about returning an unmodifiable collection?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:123: /// It's safe to modify the returned
map.
Ditto.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:139: /// not a map, or if the request
passed positional parameters rather than
Why can't the nested parameter be a list?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:147: /// Returns [index]th positional
parameter array.
This is confusingly written. How about: "Returns the element at [index] in this
array parameter."

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:150: /// or if there aren't enough
positional parameters to return the [index]th,
"indexth" -> "indexth one".

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:177: 'must be passed by position.');
Handle nesting in the error message, here and below.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
File pkg/json_rpc_2/lib/src/server.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:24: class Server {
I feel like "Server" will lead people to believe this does actual networking.
How about "Handler", "Router", or "Api"?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:47: throw new ArgumentError('There\'s already
a method named "$name".');
Would be good to support either explicit unregistration or allow this to
overwrite. (I prefer the former.)

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:65: {bool test(String name)}) {
Only passing the method name to test() is pretty limiting. Maybe you want to not
fallback on certain parameters?

How about just having a single fallback callback. If that throws something like
a FallthroughException() then it proceeds to the next fallback, otherwise it
responds with the result?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:79: if (request is! List) return
_handleSingleRequest(request);
Why support a list of requests? It seems like this makes it semantically
squishy:

1. Are the executed in parallel or sequential?
2. How are the responses organized?
3. How is an empty list handled?

How about just leaving this behavior out?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:81: return new
RpcException(error_code.INVALID_REQUEST, 'A batch must '
Serializing and returning an exception instead of throwing it feels gross here.
How about moving the .catchError() out of _handleSingleRequest() and up to here
and parseRequest and then just throw here?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:85: return
Future.wait(request.map(_handleSingleRequest)).then((results) {
Handling these in parallel might surprise users. Document.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:132: parameters = new Parameters(name,
request['params']);
Unhoist the declaration of this (or just inline it in the call).

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:187: 'be a string, but was "$method".');
Instead of quoting, how about JSON encoding method. That will handle escape
sequences in strings. Here and below.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/b...
File pkg/json_rpc_2/test/server/batch_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/b...
pkg/json_rpc_2/test/server/batch_test.dart:21: ..registerMethod('baz', (params)
=> params.getNamed('arg'));
The tests will be a bit easier to read if you give these meaningful names. How
about "foo", "id", and "arg"?

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
File pkg/json_rpc_2/test/server/invalid_request_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:21: 'Requests must be
Arrays or Objects.');
I think this error message should be singular.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:28: },
error_code.INVALID_REQUEST, 'Requests must contain a "jsonrpc" key.');
Ditto.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:37:
error_code.INVALID_REQUEST,
Nit: move this to the previous line? Here and below.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:69: test("request id must
be an Array or Object", () {
Fix description.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/p...
File pkg/json_rpc_2/test/server/parameters_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/p...
pkg/json_rpc_2/test/server/parameters_test.dart:47: 'position.'));
This is a bit confusing. How about: "passed by position" -> "an array"?

[nweiz, 2014-03-20 22:55:40]

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md
File pkg/json_rpc_2/README.md (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md#new...
pkg/json_rpc_2/README.md:64: `Server.parseRequest`:
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> How about "handleRequest" since this is the common case?

I like [parseRequest] -- it's shorter and conveys the difference between
[handleRequest] and [parseRequest] well. I'm also not sure which is going to be
the more common case. I put the web socket example first because isolates are
more of a pain to set up, but it's possible that isolate communication will be
more widely-used.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/README.md#new...
pkg/json_rpc_2/README.md:79: you can also call `Server.handleRequest` directly:
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> And then maybe make this "handleMessage" or "handleParsedRequest"?

See above.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/error_cod...
File pkg/json_rpc_2/lib/error_code.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/error_cod...
pkg/json_rpc_2/lib/error_code.dart:5: /// Error codes defined in the JSON-RPC
2.0 specificiation.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Hyperlink?

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
File pkg/json_rpc_2/lib/src/parameters.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:21: /// The underlying value of the
parameters.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> "parameters" -> "parameters object"

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:25: /// parameters map.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> How about a bit explaining how this is formatted? I assume it's like:
> 
> someArray.123.someMap.key

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:77: _getTyped(name, 'an integer', (value)
=> value is int, orElse);
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> This will do weird things in dart2js. If the value is 1.0, it will fail in the
> VM but succeed in dart2js.

I thought about only having [getNum], but I think users are going to want to
validate ints -- we do, for example. I'm fine with saying "caveat emptor" here.
I'll add a note to the documentation though.

> I guess it doesn't matter for now since this is dart:io-only.

This package is transport-independent, so it's not actually dart:io-only.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:109: /// It's safe to modify the returned
list.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> If we claim to support this, we're stuck with the performance hit of copying
> forever. That could be bad if someone uses this for something perf-heavy like
> isolate communication.
> 
> How about returning an unmodifiable collection?

Thinking this over more I think it's probably better to just let the user do
what they want. We can't protect them from mangling nested structures, and
wrapping [value] might be difficult.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:123: /// It's safe to modify the returned
map.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Ditto.

See above.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:139: /// not a map, or if the request
passed positional parameters rather than
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Why can't the nested parameter be a list?

N/A

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:147: /// Returns [index]th positional
parameter array.
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> This is confusingly written. How about: "Returns the element at [index] in
this
> array parameter."

N/A

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:150: /// or if there aren't enough
positional parameters to return the [index]th,
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> "indexth" -> "indexth one".

N/A

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:177: 'must be passed by position.');
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Handle nesting in the error message, here and below.

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
File pkg/json_rpc_2/lib/src/server.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:24: class Server {
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> I feel like "Server" will lead people to believe this does actual networking.
> How about "Handler", "Router", or "Api"?

I think it's most important to follow the spec's nomenclature here.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:47: throw new ArgumentError('There\'s already
a method named "$name".');
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Would be good to support either explicit unregistration or allow this to
> overwrite. (I prefer the former.)

I don't want to encourage people to design APIs where the methods change over
time. Users can achieve that if they want in a variety of slightly-hacky ways,
but I don't want to make it easy.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:65: {bool test(String name)}) {
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Only passing the method name to test() is pretty limiting. Maybe you want to
not
> fallback on certain parameters?
> 
> How about just having a single fallback callback. If that throws something
like
> a FallthroughException() then it proceeds to the next fallback, otherwise it
> responds with the result?

Done. I made it check for a METHOD_NOT_FOUND error.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:79: if (request is! List) return
_handleSingleRequest(request);
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Why support a list of requests?

It's in the spec: http://www.jsonrpc.org/specification#batch

> It seems like this makes it semantically
> squishy:
> 
> 1. Are the executed in parallel or sequential?

They're allowed to be executed in parallel.

> 2. How are the responses organized?

They can be returned in any order; ids are used to correlate them.

> 3. How is an empty list handled?

It's an invalid request.

> How about just leaving this behavior out?

I want to fully implement the spec.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:81: return new
RpcException(error_code.INVALID_REQUEST, 'A batch must '
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Serializing and returning an exception instead of throwing it feels gross
here.
> How about moving the .catchError() out of _handleSingleRequest() and up to
here
> and parseRequest and then just throw here?

It's important that errors be handled in [_handleSingleRequest] because of how
batches work. If a (non-INVALID_REQUEST) error occurs for a single request, that
error information needs to be included in the batch, or if the request was a
notification elided from the batch. If we move the error-handling any higher
this doesn't work.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:85: return
Future.wait(request.map(_handleSingleRequest)).then((results) {
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Handling these in parallel might surprise users. Document.

It's documented in the spec, but okay, I'll add a note at the top.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:132: parameters = new Parameters(name,
request['params']);
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Unhoist the declaration of this (or just inline it in the call).

We can't declare it until we've validated that the request is valid -- at the
declaration site, [request] might not even be a map.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:187: 'be a string, but was "$method".');
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Instead of quoting, how about JSON encoding method. That will handle escape
> sequences in strings. Here and below.

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/b...
File pkg/json_rpc_2/test/server/batch_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/b...
pkg/json_rpc_2/test/server/batch_test.dart:21: ..registerMethod('baz', (params)
=> params.getNamed('arg'));
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> The tests will be a bit easier to read if you give these meaningful names. How
> about "foo", "id", and "arg"?

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
File pkg/json_rpc_2/test/server/invalid_request_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:21: 'Requests must be
Arrays or Objects.');
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> I think this error message should be singular.

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:28: },
error_code.INVALID_REQUEST, 'Requests must contain a "jsonrpc" key.');
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Ditto.

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/i...
pkg/json_rpc_2/test/server/invalid_request_test.dart:37:
error_code.INVALID_REQUEST,
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> Nit: move this to the previous line? Here and below.

Done.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/p...
File pkg/json_rpc_2/test/server/parameters_test.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/test/server/p...
pkg/json_rpc_2/test/server/parameters_test.dart:47: 'position.'));
On 2014/03/20 18:25:58, Bob Nystrom wrote:
> This is a bit confusing. How about: "passed by position" -> "an array"?

This is another place I want to stick with the spec's language. It explicitly
defines parameters as either "by-position" or "by-name".

[Bob Nystrom, 2014-03-21 00:36:01]

A few suggestions and nits, but LGTM. This looks awesome.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
File pkg/json_rpc_2/lib/src/parameters.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/param...
pkg/json_rpc_2/lib/src/parameters.dart:109: /// It's safe to modify the returned
list.
On 2014/03/20 22:55:41, nweiz wrote:
> On 2014/03/20 18:25:58, Bob Nystrom wrote:
> > If we claim to support this, we're stuck with the performance hit of copying
> > forever. That could be bad if someone uses this for something perf-heavy
like
> > isolate communication.
> > 
> > How about returning an unmodifiable collection?
> 
> Thinking this over more I think it's probably better to just let the user do
> what they want. We can't protect them from mangling nested structures, and
> wrapping [value] might be difficult.

SGTM.

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
File pkg/json_rpc_2/lib/src/server.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:79: if (request is! List) return
_handleSingleRequest(request);
On 2014/03/20 22:55:41, nweiz wrote:
> On 2014/03/20 18:25:58, Bob Nystrom wrote:
> > Why support a list of requests?
> 
> It's in the spec: http://www.jsonrpc.org/specification#batch

Well how about that.

> 
> > It seems like this makes it semantically
> > squishy:
> > 
> > 1. Are the executed in parallel or sequential?
> 
> They're allowed to be executed in parallel.
> 
> > 2. How are the responses organized?
> 
> They can be returned in any order; ids are used to correlate them.
> 
> > 3. How is an empty list handled?
> 
> It's an invalid request.
> 
> > How about just leaving this behavior out?
> 
> I want to fully implement the spec.

SGTM!

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:132: parameters = new Parameters(name,
request['params']);
On 2014/03/20 22:55:41, nweiz wrote:
> On 2014/03/20 18:25:58, Bob Nystrom wrote:
> > Unhoist the declaration of this (or just inline it in the call).
> 
> We can't declare it until we've validated that the request is valid -- at the
> declaration site, [request] might not even be a map.

I just meant to declare the parameters variable here instead of at the top of
the method. You only use it to pass to the callback, so there's no reason to
declare it earlier, is there?

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
File pkg/json_rpc_2/lib/src/parameters.dart (right):

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:75: return new
UnmodifiableListView(value);
Do you still mean to wrap this here? You commented otherwise.

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:123: /// are represented as "[index]".
For example: "foo[0].bar.baz".
Technically, you can use any string as a key in a map, which means this would do
confusing things if you had something like a key named "foo.bar" or worse, one
with quotations or other weird characters.

If we wanted to be safe, the best answer would be to do "[${JSON.encode(key)}]",
but that's gross for the common case. Maybe check that the key only contains
alphanumerics and use ".$key" only in that case?

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:141: return '"${unquotedPath(this)}"';
Why quote it?

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:145: final wasPassed = true;
The past tense is weird. How about "isPresent" or "isAvailable" or just
"exists"?

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:196: /// [asStringOr] may be used to
provide a default value instead of rejecting the
Long line.

[nweiz, 2014-03-21 01:24:16]

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
File pkg/json_rpc_2/lib/src/server.dart (right):

https://codereview.chromium.org/205533005/diff/1/pkg/json_rpc_2/lib/src/serve...
pkg/json_rpc_2/lib/src/server.dart:132: parameters = new Parameters(name,
request['params']);
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> On 2014/03/20 22:55:41, nweiz wrote:
> > On 2014/03/20 18:25:58, Bob Nystrom wrote:
> > > Unhoist the declaration of this (or just inline it in the call).
> > 
> > We can't declare it until we've validated that the request is valid -- at
the
> > declaration site, [request] might not even be a map.
> 
> I just meant to declare the parameters variable here instead of at the top of
> the method. You only use it to pass to the callback, so there's no reason to
> declare it earlier, is there?

Oh, got it. This code used to refer to the parameters in the error handling, and
I thought it still did.

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
File pkg/json_rpc_2/lib/src/parameters.dart (right):

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:75: return new
UnmodifiableListView(value);
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> Do you still mean to wrap this here? You commented otherwise.

I do not!

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:123: /// are represented as "[index]".
For example: "foo[0].bar.baz".
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> Technically, you can use any string as a key in a map, which means this would
do
> confusing things if you had something like a key named "foo.bar" or worse, one
> with quotations or other weird characters.
> 
> If we wanted to be safe, the best answer would be to do
"[${JSON.encode(key)}]",
> but that's gross for the common case. Maybe check that the key only contains
> alphanumerics and use ".$key" only in that case?

Done.

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:141: return '"${unquotedPath(this)}"';
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> Why quote it?

It helps the parameter name stand out when it's just a single word. I've changed
this to quote it only if it's a single-level key, to avoid conflicting with
interstitial quotes as you suggested above.

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:145: final wasPassed = true;
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> The past tense is weird. How about "isPresent" or "isAvailable" or just
> "exists"?

Done.

https://codereview.chromium.org/205533005/diff/20001/pkg/json_rpc_2/lib/src/p...
pkg/json_rpc_2/lib/src/parameters.dart:196: /// [asStringOr] may be used to
provide a default value instead of rejecting the
On 2014/03/21 00:36:02, Bob Nystrom wrote:
> Long line.

Done.

[nweiz, 2014-03-21 01:24:38]

Committed patchset #3 manually as r34223 (presubmit successful).