feat(WidgetDriver): Pass Matrix API errors to the widget #4241

toger5 · 2024-11-10T22:31:26Z

Currently the WidgetDriver just returns unspecified error strings to the widget that can be used to display an issue description to the user. It is not helpful to run code like a retry or other error mitigation logic.

Here it is proposed to add standardized errors for issues that every widget driver implementation can run into (all matrix cs api errors): matrix-org/matrix-spec-proposals#2762 (comment)

This PR forwards the errors that occur during the widget processing to the widget in the correct format.

NOTE:
It does not include request Url and http Headers. See also: matrix-org/matrix-spec-proposals#2762 (comment)

Public API changes documented in changelogs (optional)

Signed-off-by:

codecov · 2024-11-13T09:16:19Z

Codecov Report

Attention: Patch coverage is 90.27778% with 7 lines in your changes missing coverage. Please review.

Project coverage is 85.20%. Comparing base (a6e1f05) to head (92e9bb3).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/matrix-sdk/src/widget/machine/mod.rs	94.11%	3 Missing ⚠️
crates/matrix-sdk/src/widget/matrix.rs	33.33%	2 Missing ⚠️
...rates/matrix-sdk/src/widget/machine/from_widget.rs	90.90%	1 Missing ⚠️
crates/matrix-sdk/src/widget/mod.rs	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4241      +/-   ##
==========================================
+ Coverage   85.17%   85.20%   +0.02%     
==========================================
  Files         280      280              
  Lines       30836    30858      +22     
==========================================
+ Hits        26266    26292      +26     
+ Misses       4570     4566       -4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bnjbvr

Makes sense! But we need many more comments for this, please; let's try to add them as we touch these files.

crates/matrix-sdk/src/widget/machine/driver_req.rs

crates/matrix-sdk/src/widget/machine/from_widget.rs

crates/matrix-sdk/src/widget/machine/incoming.rs

crates/matrix-sdk/src/widget/matrix.rs

crates/matrix-sdk/src/widget/mod.rs

crates/matrix-sdk/tests/integration/widget.rs

bnjbvr · 2024-11-28T11:59:41Z

(I might not be able to review this this week; if it's urgent, please ask review to somebody else.)

crates/matrix-sdk/src/widget/machine/from_widget.rs

crates/matrix-sdk/src/widget/machine/mod.rs

bnjbvr · 2024-12-02T15:14:18Z

crates/matrix-sdk/src/widget/machine/from_widget.rs

+    }
+
+    /// Create a error response to send to the widget from a matrix sdk error.
+    pub(crate) fn from_error(error: &Error) -> Self {


Can this take ownership of the error?

It could but would there be any benefit to that? we will only call methods that use the ref?

Avoid cloning in the function's body, and it's a good practice whenever you can do that (as it avoids future changes to this code from cloning too).

We will only call to_string and as_client_api_error on the error object. Both use &self. to_string will of course allocate new data but I am not sure there is a way to convert an error to a string without that overhead?

Indeed there's no way, but overall passing by ownership makes for cleaner APIs, whenever we can do it.

crates/matrix-sdk/src/widget/machine/mod.rs

crates/matrix-sdk/tests/integration/widget.rs

bnjbvr · 2024-12-02T15:21:12Z

crates/matrix-sdk/src/widget/machine/from_widget.rs

+    /// The matrix standard error response including the `errorcode` and the
+    /// `error` message as defined in the spec: https://spec.matrix.org/v1.12/client-server-api/#standard-error-response
+    response: Value,


Is it really worth saving the response field as a json::Value, instead of having the fields in there, automatically serialized by serde?

I think using StandardErrorBody makes sense?
This struct was a bit suspicious to me see: https://matrix.to/#/!veagCdDBjKrMsOCzrq:privacytools.io/$2ieYTtk1E4HU36rQZ7r40_bjGaWUN-zfOGz01Yfv3Ng?via=matrix.org&via=envs.net&via=kabou.ro
(I was afraid it is only supposed to be used in a very special occasion since its not even used in the ErrorBody enum)
Also the ErrorBody enum seemed to be the perfect fit here but it is not serializable.
So I did not use it before.

Would it make sense to store the result as an integer and the error message as a String?

The errorcode is a a string as well. I used the StandardErrorBody which is exactly what we expect here.

bnjbvr

This doesn't pass tests; I'd rather take a look after tests are passing, please! (or, if you need help with tests, please let me know and pinpoint where you need help :-))

….com/matrix-org/matrix-rust-sdk into toger5/widget-driver-matrix-api-errors

bnjbvr

I've addressed the comments that hadn't been addressed. Please let me know when your PR is not done yet (by removing the review request, or posting a comment), so I'm not reviewing stuff that's just about to change 🙃

Currently the WidgetDriver just returns unspecified error strings to the widget that can be used to display an issue description to the user. It is not helpful to run code like a retry or other error mitigation logic. Here it is proposed to add standardized errors for issues that every widget driver implementation can run into (all matrix cs api errors): matrix-org/matrix-spec-proposals#2762 (comment) This PR forwards the errors that occur during the widget processing to the widget in the correct format. NOTE: It does not include request Url and http Headers. See also: matrix-org/matrix-spec-proposals#2762 (comment) Co-authored-by: Benjamin Bouvier <[email protected]>

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch 2 times, most recently from 1b1a0fb to 0dd249f Compare November 13, 2024 09:01

feat(WidgetDriver): Pass matrix api errors to the widget

cccf7a0

toger5 mentioned this pull request Nov 13, 2024

MSC2762: Allowing widgets to send/receive events matrix-org/matrix-spec-proposals#2762

Open

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch from 0dd249f to cccf7a0 Compare November 13, 2024 09:18

fix clippy

6b3af4e

toger5 marked this pull request as ready for review November 13, 2024 17:09

toger5 requested a review from a team as a code owner November 13, 2024 17:09

toger5 requested review from bnjbvr and removed request for a team November 13, 2024 17:09

bnjbvr requested changes Nov 14, 2024

View reviewed changes

review

69b980f

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch from 452c513 to fd15939 Compare November 26, 2024 17:39

Merge branch 'main' into toger5/widget-driver-matrix-api-errors

6828ffa

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch from fd15939 to 6828ffa Compare November 26, 2024 17:40

remove url since we now do not include it anymore

0a5b7d3

toger5 requested a review from bnjbvr November 27, 2024 10:29

bnjbvr requested changes Dec 2, 2024

View reviewed changes

toger5 added 4 commits December 3, 2024 11:14

review

b120aa8

Merge branch 'main' into toger5/widget-driver-matrix-api-errors

90fb0e9

add with_delay mocks.

a7d0d7e

update mocks in widget.rs

2d3f2d4

toger5 requested a review from bnjbvr December 3, 2024 16:10

toger5 added 2 commits December 3, 2024 18:04

unsuse imports and comments

c2d676e

refactor response sending api

e62e013

bnjbvr requested changes Dec 4, 2024

View reviewed changes

toger5 added 2 commits December 4, 2024 13:25

fix tests

1416ff0

clippy

88ee196

bnjbvr and others added 3 commits December 4, 2024 17:16

extra review: take ownership of the errors

5ec845a

doc comments

4176d1c

Merge branch 'main' into toger5/widget-driver-matrix-api-errors

6a340f4

toger5 force-pushed the toger5/widget-driver-matrix-api-errors branch from 138be77 to 6a340f4 Compare December 4, 2024 16:33

Merge branch 'toger5/widget-driver-matrix-api-errors' of ssh://github…

92e9bb3

….com/matrix-org/matrix-rust-sdk into toger5/widget-driver-matrix-api-errors

bnjbvr approved these changes Dec 4, 2024

View reviewed changes

bnjbvr enabled auto-merge (squash) December 4, 2024 16:37

bnjbvr merged commit 111f916 into main Dec 4, 2024
39 checks passed

bnjbvr deleted the toger5/widget-driver-matrix-api-errors branch December 4, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

toger5 commented Nov 10, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

bnjbvr left a comment

bnjbvr commented Nov 28, 2024

bnjbvr Dec 2, 2024

toger5 Dec 3, 2024

bnjbvr Dec 3, 2024

toger5 Dec 3, 2024 •

edited

Loading

bnjbvr Dec 4, 2024

bnjbvr Dec 2, 2024

toger5 Dec 3, 2024

bnjbvr Dec 3, 2024

toger5 Dec 3, 2024

bnjbvr left a comment

bnjbvr left a comment

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

feat(WidgetDriver): Pass Matrix API errors to the widget #4241

Conversation

toger5 commented Nov 10, 2024 • edited Loading

codecov bot commented Nov 13, 2024 • edited Loading

Codecov Report

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toger5 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

bnjbvr left a comment

Choose a reason for hiding this comment

toger5 commented Nov 10, 2024 •

edited

Loading

codecov bot commented Nov 13, 2024 •

edited

Loading

toger5 Dec 3, 2024 •

edited

Loading