Support example API exceptions a bit better

[gnprice]

The second commit adds the example-data helper I mentioned in the review comment #1183 (comment) (/cc @PIG208). The first commit is a small prep cleanup which the second change caused me to notice.

Commit messages

6caa937 api [nfc]: Assert ZulipApiException.data is free of redundant keys

These three keys appear in the server's JSON for error responses, but
get pulled out into their own dedicated fields in ZulipApiException.
(See the field's doc, and the constructor's one non-test call site.)

The assertion is useful for tests, for keeping test data realistic.
Fix the two test cases that had missed this nuance.

de424c2 test [nfc]: Pull out an example UNAUTHORIZED API exception, and add doc

Prompted by seeing we'll need more copies of this soon:
#1183 (comment)

[chrisbobbe]

Thanks! LGTM; just an unused-import flagged by the analyzer, and one question on existing code, below.

[chrisbobbe]

I've been wondering: when we call FakeApiConnection.prepare and pass an exception, does it matter that the exception we pass is sometimes not an http.ClientException?

See the dartdoc of send on the http.BaseClient mixin, in particular the last sentence:

  /// Sends an HTTP request and asynchronously returns the response.
  ///
  /// Implementers should call [BaseRequest.finalize] to get the body of the
  /// request as a [ByteStream]. They shouldn't make any assumptions about the
  /// state of the stream; it could have data written to it asynchronously at a
  /// later point, or it could already be closed when it's returned. Any
  /// internal HTTP errors should be wrapped as [ClientException]s.
  @override
  Future<StreamedResponse> send(BaseRequest request);

and I noticed that our FakeHttpClient extends http.BaseClient.

[gnprice]

Hmm, interesting question. I don't think I'd noticed that discrepancy.

I guess one possible response is that this is still perfectly conformant with that spec, because these exceptions aren't "internal HTTP errors" and so it doesn't say anything about them. 😛 But given that the context of that doc is that the subclass is supposed to be implementing HTTP, I think the assumption is that any exception is an internal HTTP error, and so the intended meaning is that the only exceptions thrown should be [ClientException]s.

What's really happening here is that we have an exception which we want to arrange to be thrown by ApiConnection.send, and we're arranging instead for it to get thrown by the underlying http.Client.send.

… Which, when I go and look at the implementation of our ApiConnection.send, I think doesn't actually work — I think we'll end up with that method throwing a NetworkException that wraps the given ZulipApiException. That's not an accurate simulation of anything that happens in the app — what this test code looks intended to simulate is instead that http.Client.send returns normally but with a result that ApiConnection.send recognizes as an error, so that the latter throws the given ZulipApiException.

I'll investigate a bit more.

[gnprice]

One other observation is that the upstream HTTP client implementation — the one we get from http.Client() and use in the live app — doesn't seem to entirely conform to that spec either. We have the TlsException case here:

    try {
      response = await _client.send(request);
    } catch (e) {
      final String message;
      if (e is http.ClientException) {
        message = e.message;
      } else if (e is TlsException) {
        message = e.message;

because that's something we've seen empirically.

[gnprice]

Thanks for the review!

Pushed a fix for that lint error; and I have a series of new commits to address the discrepancy we learned about in the subthread above, which I think will be best handled as a follow-up PR, so I'll send that next.

[chrisbobbe]

Thanks! LGTM, merged.