split client and server channels

split client and server channels

R=brianwilkerson@google.com

Committed: https://code.google.com/p/dart/source/detail?r=33131

[Brian Wilkerson, 2014-02-27 19:06:01]

LGTM

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:78: var params =
result[Request.PARAMS];
Why did you remove the type? What other values do we expect this to have?

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:273: if (error is Map) {
What other non-null value are we guarding against (both with 'error' and with
'result')?

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:514: if (params is Map) {
What other possible values are we expecting?

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/test/cha...
File pkg/analysis_server/test/channel_test.dart (right):

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/test/cha...
pkg/analysis_server/test/channel_test.dart:14: test('invalidJsonToClient',
ChannelTest.invalidJsonToClient);
Put these in a group? I'm guessing that it will make the output easier to read.

[danrubel, 2014-02-27 20:21:14]

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:78: var params =
result[Request.PARAMS];
On 2014/02/27 19:06:02, Brian Wilkerson wrote:
> Why did you remove the type? What other values do we expect this to have?

Given valid but inappropriate JSON from the client, this could be a String. I
changed it to a var here then strengthened the if statement below to only allow
Map when run in unchecked mode.

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:273: if (error is Map) {
On 2014/02/27 19:06:02, Brian Wilkerson wrote:
> What other non-null value are we guarding against (both with 'error' and with
> 'result')?

We expect this to be a Map, but given valid JSON, it could be a String.

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:514: if (params is Map) {
On 2014/02/27 19:06:02, Brian Wilkerson wrote:
> What other possible values are we expecting?

Given valid JSON, it could be a String.

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/test/cha...
File pkg/analysis_server/test/channel_test.dart (right):

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/test/cha...
pkg/analysis_server/test/channel_test.dart:14: test('invalidJsonToClient',
ChannelTest.invalidJsonToClient);
On 2014/02/27 19:06:02, Brian Wilkerson wrote:
> Put these in a group? I'm guessing that it will make the output easier to
read.

Good point. Done.

[danrubel, 2014-02-27 20:23:44]

Committed patchset #2 manually as r33131 (presubmit successful).

[Brian Wilkerson, 2014-02-27 21:12:40]

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
File pkg/analysis_server/lib/src/protocol.dart (right):

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:78: var params =
result[Request.PARAMS];
> Given valid but inappropriate JSON from the client, this could be a String.

Ah. Then we should probably be checking the type of everything else as well.
That is, the code should read

  String id = getString(result, Request.ID);

where getString will get the property and throw an exception if it's not a
string. The exception should then be caught and an invalid request response
should be returned to the client.

https://codereview.chromium.org/182903005/diff/1/pkg/analysis_server/lib/src/...
pkg/analysis_server/lib/src/protocol.dart:436: if (data != null) {
We're not checking here to ensure that data is a Map.