Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #3366: Wrap plain-text error responses from the API in JSON #3559

Merged
merged 17 commits into from
Jul 10, 2023

Conversation

PawanHegde
Copy link
Contributor

@PawanHegde PawanHegde commented Jul 9, 2023

Summary

  • All LemmyErrors should be returned as JSON.
    • The previous behaviour was that if there was no explicit message, the message of the inner error was returned as text/plain.
  • Add a default handler for all 400-599 HTTP status responses that are not LemmyErrors.
    • This covers exceptions thrown by actix itself. The default handler for the framework returns text/plain messages.
  • 🆕 ErrorType is compulsory for creating LemmyError and "Unknown" errors need to at least have a message.
  • 🆕 The format of error response is consistent with the new LemmyErrorType changes in Error enum fixed #3487
  • 🆕 Fixed Clippy errors

Testing done

  • Valid call.


~ ❯ curl "http://localhost:8536/api/v3/comment/list?post_id=1&sort=New&type_=Local" -i                                                                                                                                                            
HTTP/1.1 200 OK
content-length: 15
access-control-expose-headers: content-type
vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
content-type: application/json
date: Mon, 10 Jul 2023 18:17:51 GMT

{"comments":[]}
  • Bad parameter. Error thrown by actix.

~ ❯ curl "http://localhost:8536/api/v3/comment/list?post_id=1&sort=New&type_=Test" -i                                                                                                                                                             
HTTP/1.1 400 Bad Request
content-length: 125
content-type: application/json
date: Mon, 10 Jul 2023 18:17:57 GMT

{"error":"unknown","message":"Query deserialize error: unknown variant `Test`, expected one of `All`, `Local`, `Subscribed`"}                                                                                                               ~ ❯                                                                                                                                                                                                                                              
  • Brought down database. Error thrown by Lemmy.

~ ❯ curl "http://localhost:8536/api/v3/comment/list?post_id=1&sort=New&type_=Local" -i                                                                                                                                                            
HTTP/1.1 400 Bad Request
content-length: 136
content-type: application/json
vary: Origin, Access-Control-Request-Method, Access-Control-Request-Headers
access-control-expose-headers: content-type
date: Mon, 10 Jul 2023 18:18:35 GMT

{"error":"unknown","message":"Error occurred while creating a new object: error connecting to server: Connection refused (os error 61)"}      

@Nutomic
Copy link
Member

Nutomic commented Jul 10, 2023

Looks good, thanks! However clippy is failing, most likely because of using unwrap. You can run ./scripts/fix-clippy.sh to check and replace them with expect or similar.

@dessalines
Copy link
Member

Seems like a little bit of bloat to add the whole middleware, but I spose its fine.

PawanHegde and others added 12 commits July 11, 2023 00:20
…myNet#3493)

* do send_activity after http response

* move to util function

* format

* fix prometheus

* make synchronous federation configurable

* cargo fmt

* empty

* empty

---------

Co-authored-by: Dessalines <dessalines@users.noreply.github.com>
added instruction to find the location of your docker directory (especially useful for those who used ansible since they never had to setup docker manually)
* Upgrade all dependencies

* as base64
* Create error type enum

* Replace magic string slices with LemmyErrorTypes

* Remove unused enum

* Add rename snake case to error enum

* Rename functions

* clippy

* Fix merge errors

* Serialize in PascalCase instead of snake_case

* Revert src/lib

* Add serialization tests

* Update translations

* Fix compilation error in test

* Fix another compilation error

* Add code for generating typescript types

* Various fixes to avoid breaking api

* impl From<LemmyErrorType> for LemmyError

* with_lemmy_type

* trigger ci

---------

Co-authored-by: SleeplessOne1917 <abias1122@gmail.com>
…eleted (LemmyNet#3543)

Two bugs were found and fixed:
- previously elements removal and deletion were counted as two separate disappearances
- removing comments did not affect post aggregations
@PawanHegde
Copy link
Contributor Author

PawanHegde commented Jul 10, 2023

Thanks for the reviews. I've had to update some code because of the changes in #3487. I've updated the summary as well.

@Nutomic Nutomic merged commit ef9dc5d into LemmyNet:main Jul 10, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants