Add error to negotiate #2998
Add error to negotiate #2998
Conversation
| private JsonParser jsonParser = new JsonParser(); | ||
|
|
||
| public NegotiateResponse(String negotiatePayload) { | ||
| JsonObject negotiateResponse = jsonParser.parse(negotiatePayload).getAsJsonObject(); | ||
| if (negotiateResponse.has("error")) { | ||
| this.error = negotiateResponse.get("url").getAsString(); | ||
| return; |
There was a problem hiding this comment.
I don't think the parsing logic should exit early like this. You're right that an error property generally means everything else should be ignored, but let the caller decide that.
| @@ -428,6 +428,10 @@ private async Task<NegotiationResponse> NegotiateAsync(Uri url, HttpClient httpC | |||
| { | |||
| negotiateResponse = NegotiateProtocol.ParseResponse(responseStream); | |||
| } | |||
| if (!string.IsNullOrEmpty(negotiateResponse.Error)) | |||
| { | |||
| throw new InvalidOperationException(negotiateResponse.Error); | |||
There was a problem hiding this comment.
InvalidOperationException isn't really right here... We don't know anything about the underlying problem, it's no better than just throwing Exception.
specs/TransportProtocols.md
Outdated
| @@ -62,6 +62,19 @@ The `POST [endpoint-base]/negotiate` request is used to establish a connection b | |||
| * The `url` which is the URL the client should connect to. | |||
| * The `accessToken` which is an optional bearer token for accessing the specified url. | |||
|
|
|||
|
|
|||
| 3. A response that contains an `error` which should abort the connection attempt. | |||
There was a problem hiding this comment.
Maybe we need a protocol version here @anurse
There was a problem hiding this comment.
Yeah, it's not a bad idea. But do we want to do the two-part handshake that ASP.NET SignalR does? Where the client reports it's max supported version?
There was a problem hiding this comment.
ERRR, I hope we don't need to change it that much.
|
No |
| return ResponseUtils.CreateResponse(HttpStatusCode.OK, | ||
| JsonConvert.SerializeObject(new | ||
| { | ||
| error = "Negotiate failed." |
There was a problem hiding this comment.
nit: Change this error message to something that isn't about negotiation. This seems like something that could be ambiguous
| this.connectionId = reader.nextString(); | ||
| break; | ||
| default: | ||
| // Skip unknown property, allows new clients to still work with old protocols |
There was a problem hiding this comment.
Do we have a test case for this assertion?
There was a problem hiding this comment.
+have a test case for skipping a simple value, e.g. "a string!", and a complex value, e.g. ["array", "of", "content"]
I'm guessing skipValue will skip all of the complex value but it is worth double checking. It is a common bug I've seen in code that manually reads JSON
There was a problem hiding this comment.
We do now, both cases. Thanks James
|
@dotnet-bot test Windows Release Spanish Language x64 Build |
#2988
No httpclient testability in Java yet.