Websocket connect, clear error response

[philos3]

Update websocket_impl.dart

It is usefull with more detail for "Connection was not upgraded to websocket error "

---

• I’ve reviewed the contributor guide and applied the relevant portions to this PR.

Contribution guidelines:

• See our contributor guide for general expectations for PRs.
• Larger or significant changes should be discussed in an issue before creating a PR.
• Contributions to our repos should follow the Dart style guide and use dart format.

Note that this repository uses Gerrit for code reviews. Your pull request will be automatically converted into a Gerrit CL and a link to the CL written into this PR. The review will happen on Gerrit but you can also push additional commits to this PR to update the code review.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[devoncarew]

@philos3 - thanks for the contribution! Before we can accept it you'll need to sign the CLA at https://cla.developers.google.com/.

Also, can you update the description of the PR to describe more what this PR is trying to accomplish. cc @brianquinlan

[copybara-service]

Thank you for your contribution! This project uses Gerrit for code reviews. Your pull request has automatically been converted into a code review at:

https://dart-review.googlesource.com/c/sdk/+/313042

Please wait for a developer to review your code review at the above link; you can speed up the review if you sign into Gerrit and manually add a reviewer that has recently worked on the relevant code. See CONTRIBUTING.md to learn how to upload changes to Gerrit directly.

Additional commits pushed to this PR will update both the PR and the corresponding Gerrit CL. After the review is complete on the CL, your reviewer will merge the CL (automatically closing this PR).

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

[brianquinlan]

I left some feedback at https://dart-review.googlesource.com/c/sdk/+/313042

[mraleph]

@philos3 would you like to address @brianquinlan's comments or should we close this PR?

[brianquinlan]

@mraleph Could we keep this open for now? Let's say that, if @philos3 isn't responsive by Monday, then I'll complete this change.

[JoaoVitorAmorim]

Approved @philos3

[mraleph]

@mraleph Could we keep this open for now? Let's say that, if @philos3 isn't responsive by Monday, then I'll complete this change.

The deadline has passed, so it is on your plate now :)

[brianquinlan]

It's on my plate. Adding the messages was easy - writing tests for all the failure variations is hard ;-)

[mit-mit]

Any updates @brianquinlan ?

[vsmenon]

gentle ping

[copybara-service]

https://dart-review.googlesource.com/c/sdk/+/313042 has been updated with the latest commits from this pull request.

[brianquinlan]

@mit-mit @vsmenon

Is this a priority? The original PR doesn't have tests so isn't mergable as-is. I can write the tests myself if getting this in is important.

[mit-mit]

Given we're just updating a status message, perhaps we can exempt this one. wdyt Vijay?

[brianquinlan]

The PR outputs the entire response message in the error - this could be arbitrarily large.

I have a modified version of the PR that decodes the error in more detail without including the entire response body:

-      if (response.statusCode != HttpStatus.switchingProtocols ||
-          connectionHeader == null ||
-          !connectionHeader.any((value) => value.toLowerCase() == "upgrade") ||
-          response.headers.value(HttpHeaders.upgradeHeader)!.toLowerCase() !=
-              "websocket") {
-        return error("Connection to '$uri' was not upgraded to websocket");
-      }
+      if (response.statusCode != HttpStatus.switchingProtocols) {
+        return error("Unexpected status code: ${response.statusCode}");
+      }
+      if (connectionHeader == null) {
+        return error("Response did not contain a 'connection' header");
+      }
+      if (connectionHeader.first.toLowerCase() != "upgrade") {
+        return error(
+            "Unexpected 'connection' header: ${connectionHeader.first}");
+      }
+     if (upgradeHeader == null) {
+       return error("Response did not contain an 'upgrade' header");
+     }
+     if (upgradeHeader.first.toLowerCase() != "websocket") {
+       return error("Unexpected 'upgrade' header: ${upgradeHeader.first}");
+     }
+     if (acceptHeader == null) {
+       return error(
+           "Response did not contain a 'Sec-WebSocket-Accept' header");
+     }

But writing tests for this is not trivial and it didn't seem worth it. @mit-mit if there is some reason why this should be a priority (e.g., people are complaining about error messages) then just let me know.

[mraleph]

I assume this is stale. Please reopen if not.