showing all error when publishing fails #1327
Conversation
src/registry/routes/publish.ts
Outdated
| @@ -65,20 +65,20 @@ export default function publish(repository: Repository) { | |||
| res.status(200).json({ ok: true }); | |||
| } catch (err: any) { | |||
| if (err.code === 'not_allowed') { | |||
| res.errorDetails = `Publish not allowed: ${err.msg}`; | |||
| res.status(403).json({ error: err.msg }); | |||
| res.errorDetails = `Publish not allowed: ${err}`; | |||
There was a problem hiding this comment.
I think the proposed solutions included more lines to transform the errormessage
There was a problem hiding this comment.
Hey Ricardo,
I should have mentioned it while raising the PR.
Anyways in the proposed solution
I was checking the error object if it has msg field or message field, if none then I was JSON.stringifying the object and setting it everywhere
But it made sense to me, to just send the whole error object.
Let me know if you think otherwise or why should we not do this.
There was a problem hiding this comment.
Well I'm thinking that we probably shouldn't return a stack trace on a production environment API
There was a problem hiding this comment.
Makes sense.
I have made the changes that match the same as I had proposed in the issue.
There was a problem hiding this comment.
Do you think we should also include logging errors ?
There was a problem hiding this comment.
I think maybe we can base it on res.conf.local to decide if we pass the whole thing or not
There was a problem hiding this comment.
Do you think, will it be better to use res.conf.debug instead of local?
There was a problem hiding this comment.
There is no conf.debug property. Local should be fine.
There was a problem hiding this comment.
Cool, I have made the changes
Sending whole error log only when `err.msg` and `err.message` fields are not present in error object
|
LGTM |
Relates to #1326
Explain the details for making this change. What existing problem does the pull request solve?
This is to avoid empty error responses as mentioned in issue.
closes #1326.