api: Include stack trace when reporting MalformedServerResponseException #1374

lakshya1goel · 2025-02-23T07:18:16Z

Changes:

Add causeStackTrace field to MalformedServerResponseException
Include stack trace in the exception's toString() output
Add test to verify stack trace is included in error messages

Testing:

Added unit test that verifies stack trace is included when a type mismatch occurs
Manually tested with registerQueue to confirm stack trace shows in error dialog

Screenshot of error

gnprice · 2025-02-24T21:21:36Z

It doesn't look like this new parameter causeStackTrace ever gets passed to the constructor. So it doesn't actually do anything in the live app.

Please test your change end to end in the app, and revise it so it solves the problem described in the issue:

When a request to the server fails, we typically show an error to the user. (The main gap in that is #890, which we should fix.) The details of the error can be useful, particularly if the user takes a screenshot of them to include in reporting the issue to us.

If the cause of the error is that the server's response doesn't match our expectations — a MalformedServerResponseException — then a key thing we want to know is where in the schema the mismatch occurred: what field of what type of object. […] So we should show the stack trace when reporting a MalformedServerResponseException.

lakshya1goel · 2025-03-01T11:53:53Z

Hi @PIG208, whenever you get a chance, could you please take a look at my PR for an initial review? Thanks!

PIG208

Thanks for working on this! Left some comments.

PIG208 · 2025-03-03T19:04:08Z

test/api/exception_test.dart

+      wrongType['properties']! as Map<String, dynamic>;
+      fail('Should have thrown');
+    } catch (e, stackTrace) {
+      final exception = MalformedServerResponseException(


A more end-to-end test will be more valuable. MalformedServerResponseException already has a bunch of tests in test/api/core_test.dart.

We should then leave a comment similar to this one above:

// NetworkException.toString: see "API network errors" test in core_test.dart

to indicate that the tests live elsewhere.

Bumping this. I meant that we should test MalformedServerResponseException.toString in test/api/core_test.dart, removing the test in test/api/exception_test.dart, and adding the comment in test/api/exception_test.dart indicating that the test for MalformedServerResponseException.toString lives in core_test.dart.

The other comment that you moved in the new revision was meant to be at where it was earlier, since it is a placeholder indicating that a test would be there when it isn't; it's not a description of the test.

PIG208 · 2025-03-03T19:06:37Z

lib/api/exception.dart


  MalformedServerResponseException({
    required super.routeName,
    required super.httpStatus,
    required super.data,
    this.causeException,
+    this.causeStackTrace
  }) : super(message: causeException == null
         ? GlobalLocalizations.zulipLocalizations
            .errorMalformedResponse(httpStatus)
         : GlobalLocalizations.zulipLocalizations
            .errorMalformedResponseWithCause(
              httpStatus, causeException.toString()));


We already stringify causeException and make it a part of message. Including it again in the toString implementation will repeat that in the error message. This is what I got when printing out the error from the test:

MalformedServerResponseException: 200 registerQueue: Server gave malformed response; HTTP status 200; type 'String' is not a subtype of type 'Map<String, dynamic>' in type cast Caused by: type 'String' is not a subtype of type 'Map<String, dynamic>' in type cast #0 main.<anonymous closure> (file:///home/zixuan/zulip-stuff/zulip-flutter/test/api/exception_test.dart:49:32) #1 Declarer.test.<anonymous closure>.<anonymous closure> (package:test_api/src/backend/declarer.dart:229:19) <asynchronous suspension> #2 Declarer.test.<anonymous closure> (package:test_api/src/backend/declarer.dart:227:7) <asynchronous suspension> #3 Invoker._waitForOutstandingCallbacks.<anonymous closure> (package:test_api/src/backend/invoker.dart:258:9) <asynchronous suspension> true

PIG208 · 2025-03-03T19:13:21Z

lib/api/exception.dart

+    sb.write(" $routeName");
+    sb.write(": $message");
+    if (causeException != null) {
+      sb.write('\nCaused by: $causeException');


New strings added should be localized. We already have errorMalformedResponseWithCause and errorMalformedResponse, so it might be a good idea to reuse them/add one.

Hmm, I see that you added "Caused by" back. It's an English phrase that should be translated.

PIG208 · 2025-03-03T19:16:35Z

lib/api/exception.dart

+    sb.write(": $message");
+    if (causeException != null) {
+      sb.write('\nCaused by: $causeException');
+      if (causeStackTrace != null) {


Looks like it would be a developer error if causeException is present but causeStackTrace is not. Let's have an assertion in the constructor to validate that.

lakshya1goel · 2025-03-09T14:17:11Z

Hi @PIG208, thanks for the above reviews. I have pushed the revision. PTAL.

PIG208 · 2025-03-10T19:33:00Z

assets/l10n/app_en.arb

@@ -552,6 +552,24 @@
      "details": {"type": "String", "example": "type 'Null' is not a subtype of type 'String' in type cast"}
    }
  },
+  "errorMalformedResponseFormat": "{className}: {httpStatus} {routeName}: {message}",
+  "@errorMalformedResponseFormat": {
+    "description": "Format for error messages from malformed server responses. The message parameter is already localized.",


nit: the audience of the string description is mainly the translators; so there is nothing actionable for them knowing that message is localized. Similarly, we can leave out "format" in both the name of the string and its description.

How about naming it "errorMalformedServerResponse"? For rewriting the description, you can find soem examples with:

git grep -hEA2 'error.+\",' assets/l10n/app_en.arb

You can check out git grep --help for what each of these options mean.

PIG208 · 2025-03-10T19:35:32Z

assets/l10n/app_en.arb

+  "errorMalformedServerResponseExceptionAssertMessage": "If causeException is provided, causeStackTrace must also be provided",
+  "@errorMalformedServerResponseExceptionAssertMessage": {
+    "description": "Developer error message when creating a MalformedServerResponseException with incomplete error details"
+  },


Because assertions are only run in debug builds, messages from them are usually only visible to the developer. We can skip translating those strings.

PIG208 · 2025-03-10T19:37:26Z

assets/l10n/app_en.arb

+  "errorMalformedServerResponseExceptionName": "MalformedServerResponseException",
+  "@errorMalformedServerResponseExceptionName": {
+    "description": "Name of the error type for malformed server responses"
+  },


Hmm, it doesn't seem that a class name that is only meaningful to our codebase can be translated.

PIG208 · 2025-03-10T19:48:00Z

lib/api/exception.dart

+    final zulipLocalizations = GlobalLocalizations.zulipLocalizations;
+    return zulipLocalizations.errorMalformedResponseFormat(
+      zulipLocalizations.errorMalformedServerResponseExceptionName,
+      httpStatus,
+      routeName,
+      message);


Looks like the stack trace is not used in this revision. We should include it in the error message.

I think the structure we had in the previous revision is good. #1374 (comment) was for the untranslated "Cause by". Since we have removed it in favor of errorMalformedResponseWithCause, the remaining parts of the error message probably don't need translations.

PIG208 · 2025-03-10T19:49:10Z

Thanks for the update! Left some comments.

lakshya1goel · 2025-03-18T14:48:56Z

Hi @PIG208, thanks for the review. I have pushed the revision. PTAL!

lakshya1goel · 2025-03-21T14:34:15Z

Hi @PIG208 , whenever you get some time, please take a look at the PR. Thanks!

PIG208

Thanks for the update! Left some more comments

PIG208 · 2025-03-20T01:05:16Z

lib/api/exception.dart

+  }) : assert(causeException == null || causeStackTrace != null,
+         'If causeException is provided, causeStackTrace must also be provided'),


This doesn't cover the case when causeException is missing and causeStackTrace is given. How about something like (causeException == null && causeStackTrace == null) || (causeException != null && causeStackTrace != null)?

PIG208 · 2025-03-21T21:24:16Z

lib/api/exception.dart

+    sb.write(" $routeName");
+    sb.write(": $message");
+    if (causeException != null) {
+      sb.write('\nCaused by: $causeException');


Hmm, I see that you added "Caused by" back. It's an English phrase that should be translated.

PIG208 · 2025-03-21T21:30:01Z

lib/api/exception.dart

+  @override
+  String toString() {
+    final StringBuffer sb = StringBuffer('MalformedServerResponseException');
+    sb.write(" $routeName");
+    sb.write(": $message");
+    if (causeException != null) {
+      sb.write('\nCaused by: $causeException');
+      sb.write('\n$causeStackTrace');
+    }
+    return sb.toString();
+  }


Hmm, I see that you added "Caused by" back. It's an English phrase that should be translated. However, maybe we don't need to override toString at all? ZulipApiException overrides it because it has multiple checks on fields like httpStatus, code, and data. But for MalformedServerResponseException, we are only checking causeException.

The original code seems capable of handling this:

MalformedServerResponseException({ required super.routeName, required super.httpStatus, required super.data, this.causeException, }) : super(message: causeException == null ? GlobalLocalizations.zulipLocalizations .errorMalformedResponse(httpStatus) : GlobalLocalizations.zulipLocalizations .errorMalformedResponseWithCause( httpStatus, causeException.toString()));

We just need to add the stack trace to the case where causeException is non-null, and have the assertions here to ensure that both of these fields are non-null or null at the same time.

PIG208 · 2025-03-21T21:34:19Z

test/api/core_test.dart

+    try {
+      throw FormatException("Expected field 'stream_id' to be an integer, but got string 'abc123'");
+    } catch (e, st) {
+      final exception = MalformedServerResponseException(


Hmm, it would be ideal for the MalformedServerResponseException to be thrown by non-test code, as this is the point of having this test in core_test.dart.

I think an existing test, "malformed API success responses: exception preserves details", has all the setup needed to test this correctly.

Thanks for pointing. So should I update the exiting test only to check for the causeStackTrace as well or need to add a separate test for it?

Updated test can be:

test('malformed API success responses: exception preserves details', () async { int distinctivelyNamedFromJson(Map<String, dynamic> json) { throw DistinctiveError("something is wrong"); } try { await tryRequest(json: {}, fromJson: distinctivelyNamedFromJson); assert(false); } catch (e, st) { check(e).isA<MalformedServerResponseException>() ..causeException.isA<DistinctiveError>() ..message.contains("something is wrong") ..has((it) => it.causeStackTrace, "causeStackTrace").isNotNull() ..has((it) => it.causeStackTrace.toString(), "causeStackTrace.toString()") .contains("distinctivelyNamedFromJson"); check(st.toString()).contains("distinctivelyNamedFromJson"); } });

Updating the existing test sounds good to me. I would add causeStackTrace to the …Checks extension of the error, to avoid inlining the has' here.

lakshya1goel · 2025-03-25T18:18:21Z

Hi @PIG208, I have pushed the revision PTAL, Thanks!

PIG208

Thanks for the update! Could you also include a screenshot of what an error message would look like with the next revision? Left some comments.

PIG208 · 2025-03-29T00:58:12Z

lib/api/exception.dart

+  }) : assert((causeException == null && causeStackTrace == null) ||
+              (causeException != null && causeStackTrace != null),
+         'causeException and causeStackTrace must either both be null or both be non-null'),
+       super(message: causeException == null
         ? GlobalLocalizations.zulipLocalizations
            .errorMalformedResponse(httpStatus)
         : GlobalLocalizations.zulipLocalizations


Looks like this still needs to add the stack trace to the error message below.

PIG208 · 2025-03-29T00:58:19Z

lib/api/exception.dart


  MalformedServerResponseException({
    required super.routeName,
    required super.httpStatus,
    required super.data,
    this.causeException,
-  }) : super(message: causeException == null
+    this.causeStackTrace


nit: trailing comma

PIG208 · 2025-03-29T00:59:02Z

test/api/core_test.dart

@@ -425,7 +425,8 @@ void main() {
    } catch (e, st) {
      check(e).isA<MalformedServerResponseException>()
        ..causeException.isA<DistinctiveError>()
-        ..message.contains("something is wrong");
+        ..message.contains("something is wrong")
+        ..causeStackTrace.toString().contains("distinctivelyNamedFromJson");


We should check that the stack trace is a part of the error message, so it is visible in the error shown to the user.

This looks better now, one nit is that we want to make sure that the string to match against the stack trace is uniquely present in the stack trace. I think
"distinctivelyNamedFromJson" is a bit dubious identifier since the error message might reasonable contain it.

How about just the first line of the actual stack trace without the path, i.e., "#0 main..distinctivelyNamedFromJson"?

Calling toString() on a Subject also causes this to not fail when the expected substring is not found, because we intended to call Subject.contains, but this actually calls String.contains, which just returns a bool and does not throw.

lakshya1goel · 2025-04-02T17:07:45Z

Hi @PIG208, I have pushed the revision as well as updates the PR description with the screenshot of error. PTAL, thanks!

PIG208

Thanks for the update! I think the test still needs some tweaks, but it looks a lot closer now.

One other note: I think I wasn't clear in my previous #1374 (review) that I was referring to taking a screenshot of the error the user might see while using the app.

PIG208 · 2025-04-07T19:51:51Z

test/api/exception_checks.dart

@@ -28,4 +28,5 @@ extension Server5xxExceptionChecks on Subject<Server5xxException> {

 extension MalformedServerResponseExceptionChecks on Subject<MalformedServerResponseException> {
  Subject<Object?> get causeException => has((e) => e.causeException, 'causeException');
+  Subject<String> get causeStackTrace => has((e) => e.causeStackTrace.toString(), 'causeStackTrace');


The type should match the actual property:

Suggested change

Subject<String> get causeStackTrace => has((e) => e.causeStackTrace.toString(), 'causeStackTrace');

Subject<StackTrace?> get causeStackTrace => has((e) => e.causeStackTrace, 'causeStackTrace');

And then, we can have StackTraceChecks next to ErrorChecks in test/stdlib_checks.dart that offers asString on Subject<StackTrace>.

PIG208 · 2025-04-07T19:56:28Z

test/api/core_test.dart

@@ -425,7 +425,8 @@ void main() {
    } catch (e, st) {
      check(e).isA<MalformedServerResponseException>()
        ..causeException.isA<DistinctiveError>()
-        ..message.contains("something is wrong");
+        ..message.contains("something is wrong")
+        ..causeStackTrace.toString().contains("distinctivelyNamedFromJson");


This looks better now, one nit is that we want to make sure that the string to match against the stack trace is uniquely present in the stack trace. I think
"distinctivelyNamedFromJson" is a bit dubious identifier since the error message might reasonable contain it.

How about just the first line of the actual stack trace without the path, i.e., "#0 main..distinctivelyNamedFromJson"?

PIG208 · 2025-04-07T20:07:17Z

test/api/core_test.dart

@@ -425,7 +425,8 @@ void main() {
    } catch (e, st) {
      check(e).isA<MalformedServerResponseException>()
        ..causeException.isA<DistinctiveError>()
-        ..message.contains("something is wrong");
+        ..message.contains("something is wrong")
+        ..causeStackTrace.toString().contains("distinctivelyNamedFromJson");


Calling toString() on a Subject also causes this to not fail when the expected substring is not found, because we intended to call Subject.contains, but this actually calls String.contains, which just returns a bool and does not throw.

Fixes: zulip#1083

lakshya1goel · 2025-04-15T06:26:45Z

One other note: I think I wasn't clear in my previous #1374 (review) that I was referring to taking a screenshot of the error the user might see while using the app.

Should I add a snackbar or dailogbox in the app to show error to the user on the app screen?

PIG208 · 2025-04-15T20:15:11Z

I think we shouldn't need to add a snack bar/dialog, since the error should have been handled from the polling loop in UpdateMachine.

Such an error might look like this:

PIG208

Thanks for the update! Marking this for Greg's review.

PIG208 · 2025-04-15T20:37:38Z

test/stdlib_checks.dart

@@ -30,6 +30,10 @@ extension ErrorChecks on Subject<Error> {
  Subject<String> get asString => has((x) => x.toString(), 'toString'); // TODO(checks): what's a good convention for this?
 }

+extension StackTraceChecks on Subject<StackTrace> {
+  Subject<String> get asString => has((x) => x.toString(), 'toString'); 


nit:

Suggested change

Subject<String> get asString => has((x) => x.toString(), 'toString');

Subject<String> get asString => has((x) => x.toString(), 'toString'); // TODO(checks): what's a good convention for this?

(removed trailing space and copied the comment from the other asString check)

gnprice · 2025-04-15T21:06:44Z

As @PIG208 said above, I think we shouldn't need to add any new snack bar or dialog in order for the error to get shown to the user — the existing error dialog should do it.

But as the issue #1083 says, and as I emphasized in #1374 (comment) at the top of this thread, the point of the issue is about what information we show to the user.

So, does this PR cause the stack trace to get shown to the user when they hit one of these errors? Please post a screenshot of what that looks like.

(The PR description currently has a screenshot of something shown in your terminal, as a developer. That isn't informative about what the user sees. If it were relevant information, then copy-paste into a code block would be a clearer way to present it than a screenshot.)

lakshya1goel mentioned this pull request Feb 23, 2025

Include stack trace when reporting MalformedServerResponseException #1083

Open

lakshya1goel force-pushed the issue1083 branch from ffb0a30 to 4a5a7c9 Compare February 27, 2025 17:49

PIG208 self-assigned this Mar 1, 2025

PIG208 reviewed Mar 3, 2025

View reviewed changes

lakshya1goel force-pushed the issue1083 branch from 4a5a7c9 to 742ac1e Compare March 9, 2025 14:08

lakshya1goel requested a review from PIG208 March 9, 2025 14:16

PIG208 reviewed Mar 10, 2025

View reviewed changes

lakshya1goel force-pushed the issue1083 branch from 742ac1e to fb71578 Compare March 18, 2025 14:35

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Mar 21, 2025

PIG208 reviewed Mar 21, 2025

View reviewed changes

lakshya1goel force-pushed the issue1083 branch 2 times, most recently from 7e205d4 to 56e39b8 Compare March 25, 2025 17:46

lakshya1goel requested a review from PIG208 March 25, 2025 18:17

PIG208 reviewed Mar 29, 2025

View reviewed changes

lakshya1goel force-pushed the issue1083 branch from 56e39b8 to 12ffdb4 Compare April 2, 2025 17:06

lakshya1goel force-pushed the issue1083 branch from 12ffdb4 to bd51406 Compare April 2, 2025 17:39

lakshya1goel requested a review from PIG208 April 3, 2025 15:22

PIG208 reviewed Apr 7, 2025

View reviewed changes

api: Include stack trace when reporting MalformedServerResponseException

b8757b8

Fixes: zulip#1083

lakshya1goel force-pushed the issue1083 branch from bd51406 to b8757b8 Compare April 15, 2025 06:05

lakshya1goel requested a review from PIG208 April 15, 2025 06:26

PIG208 reviewed Apr 15, 2025

View reviewed changes

PIG208 added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Apr 15, 2025

PIG208 assigned gnprice and unassigned PIG208 Apr 15, 2025

PIG208 requested a review from gnprice April 15, 2025 20:38

		}) : assert(causeException == null \|\| causeStackTrace != null,
		'If causeException is provided, causeStackTrace must also be provided'),

	Subject<String> get causeStackTrace => has((e) => e.causeStackTrace.toString(), 'causeStackTrace');
	Subject<StackTrace?> get causeStackTrace => has((e) => e.causeStackTrace, 'causeStackTrace');

	Subject<String> get asString => has((x) => x.toString(), 'toString');
	Subject<String> get asString => has((x) => x.toString(), 'toString'); // TODO(checks): what's a good convention for this?

api: Include stack trace when reporting MalformedServerResponseException #1374

Are you sure you want to change the base?

api: Include stack trace when reporting MalformedServerResponseException #1374

Uh oh!

Conversation

lakshya1goel commented Feb 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Testing:

Screenshot of error

Uh oh!

gnprice commented Feb 24, 2025

Uh oh!

lakshya1goel commented Mar 1, 2025

Uh oh!

PIG208 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshya1goel commented Mar 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PIG208 commented Mar 10, 2025

Uh oh!

lakshya1goel commented Mar 18, 2025

Uh oh!

lakshya1goel commented Mar 21, 2025

Uh oh!

PIG208 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshya1goel commented Mar 25, 2025

Uh oh!

PIG208 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lakshya1goel commented Apr 2, 2025

Uh oh!

lakshya1goel commented Feb 23, 2025 •

edited

Loading

PIG208 left a comment •

edited

Loading

PIG208 commented Apr 15, 2025 •

edited

Loading