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

Error message returned from the server is not shown to the user #4079

Closed
2 tasks done
yaegor opened this issue Sep 1, 2022 · 11 comments
Closed
2 tasks done

Error message returned from the server is not shown to the user #4079

yaegor opened this issue Sep 1, 2022 · 11 comments
Assignees
Labels
Bug Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3)

Comments

@yaegor
Copy link
Contributor

yaegor commented Sep 1, 2022

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

I use @uppy/aws-s3-multipart and when my custom server in response to the initial POST at companionUrl returns 400 response with JSON like {"error":true,"message":"Session is invalid. Reload the page and start again."}, the message is not shown to the user anywhere.

Expected behavior

It would be great to include the error message from the server into the message shown to the user.

Actual behavior

Uppy dashboard, on click on (?) displays: "Upload failed / Could not post <my_server_url>" which is not very helpful.

yaegor added a commit to yaegor/uppy that referenced this issue Sep 1, 2022
@yaegor
Copy link
Contributor Author

yaegor commented Sep 1, 2022

A change like this would fix the issue for me, but I am not sure how appropriate it is.

@yaegor yaegor changed the title Error message returned from the server is not shown to the user (aws-s3-multipart) Error message returned from the server is not shown to the user Sep 1, 2022
@yaegor
Copy link
Contributor Author

yaegor commented Sep 1, 2022

BTW, it would be great to provide ability to override the error messages (e.g. I'd drop that "Could not post" part of the default message at all)

@Murderlon
Copy link
Member

Hi!

It would be great to include the error message from the server into the message shown to the user.

That would be a security concern. We can't blindly return any error to the client from S3, chances are we would be sending sensitive data to the client. This was a deliberate choice when making Companion.

Closing this. But we can continue to discuss what you are particularly trying to inform the user about and if there are other ways of doing so.

@Murderlon Murderlon closed this as not planned Won't fix, can't repro, duplicate, stale Sep 2, 2022
@yaegor
Copy link
Contributor Author

yaegor commented Sep 2, 2022

Hi!
Thank you for the comment, it makes sense.
However, the case I described is different: it is companion server returning an error message to the client in the response. The message is already on the client, it is just not shown to the user in UI.

@Murderlon
Copy link
Member

I see, I mistakenly thought your suggestion was in the server-side request. Sorry about that. @mifi I think we can do this then right?

@Murderlon Murderlon reopened this Sep 2, 2022
@mifi
Copy link
Contributor

mifi commented Sep 2, 2022

I'm not sure if Error messages should be shown directly to the user, because:

  1. we should try to engineer away unhandled errors and instead handle them and show an appropriate message
  2. error messages from a provider (or custom server) are often not i18n, so they shouldn't be shown to a user who expects the interface to be in their language.

What do you think?

@yaegor
Copy link
Contributor Author

yaegor commented Sep 2, 2022

Thank you for the considerations.

  1. My main concern is custom companion server being able to show error messages to the user. Returning JSON like {"error":true,"message":"..."} seems to be quite suitable way of passing the message to the client. These are probbaly not unhandled errors (if they are, it's the server which should probably be fixed). The message should then be passed to UI not being mixed with the unhandled errors - this is right. In this light the above linked simplistic change is most probbaly not appropriate.
  2. It seems that i18n should be directly supported by the companion API (e.g. the server should generate the messages in the locale passed in a request header). Then it will be up to the server to supply the right message.

Just to clarify the issue: right now if an error occurs on the companion server side during an operation, user is presented with "Could not get/post/... " message which does not provide much help to the user even if the server if fully aware of the error details. Noting to the user for example if it makes any sense to retry the operation would improve the usability.

@mifi
Copy link
Contributor

mifi commented Sep 4, 2022

  • My main concern is custom companion server being able to show error messages to the user. Returning JSON like {"error":true,"message":"..."} seems to be quite suitable way of passing the message to the client. These are probbaly not unhandled errors (if they are, it's the server which should probably be fixed). The message should then be passed to UI not being mixed with the unhandled errors - this is right. In this light the above linked simplistic change is most probbaly not appropriate.

Yes, I think we should ideally separate expected and unexpected errors. Expected errors should be presented to the user as i18n error messages with something actionable. Unexpected errors should just say something like "something went wrong, try again".

2. It seems that i18n should be directly supported by the companion API (e.g. the server should generate the messages in the locale passed in a request header). Then it will be up to the server to supply the right message.

Yes, that's one way of doing it, by letting Uppy send the locale for example as a HTTP header to all companion requests (I believe all browsers already do that, assuming that Uppy always uses the same locale as the user's browser). Then companion needs to implement i18n.

The other way of doing it (arguably cleaner) is that Companion will never send any error messages back to the Uppy client - only error codes. Then the client (which already has i18n support) will lookup the error codes and find a corresponding i18n key and display the error.

If we solve it in the first way, Companion would also have to forward the locale to all providers (for example Google Drive, or your custom aws-s3-multipart server), and the servers would have to respect the locale when returning error messages. This also assumes that all those providers/servers support i18n and support all the same locales as uppy does. So I think this is the biggest argument for using error codes instead.

Just to clarify the issue: right now if an error occurs on the companion server side during an operation, user is presented with "Could not get/post/... " message which does not provide much help to the user even if the server if fully aware of the error details. Noting to the user for example if it makes any sense to retry the operation would improve the usability.

Yea regardless, we should definitely improve this! It's a horrible error to show the user. I'll see if I can cook up a PR

@yaegor
Copy link
Contributor Author

yaegor commented Sep 5, 2022

Hi Mikael,
Just some related considerations:
In my specific case, I implement a server which is mimicking companion S3-related API. The server has all the details and can generate a meaningful message to the user. The messages are specific to my implementation, so I will need to use custom error codes and provide own messages on the client side.
However, in case of the default companion implementation the error codes and canned messages on the client can work ok as long as they are static strings not including any variables.

@Murderlon Murderlon added Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3) and removed Triage labels Oct 19, 2022
@rajjanorkar
Copy link

I am also facing an similar issue, errors returned from server is not shown to users.

@mifi
Copy link
Contributor

mifi commented Aug 29, 2024

closing in favor of #5436

@mifi mifi closed this as not planned Won't fix, can't repro, duplicate, stale Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3)
Projects
None yet
Development

No branches or pull requests

4 participants