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]: 🐛 Strapi parser middleware, catch block error logging #9612

Merged

Conversation

ognjenjevremovic
Copy link
Contributor

What does it do?

Throwing error inside of a controller (using plain javascript) results in a different error in parser middleware.
Before calling (String.prototype.includes) method on the message property on the error object, a check should be performed to see if the message property is defined on the error instance so no runtime TypeError is thrown.

Why is it needed?

Throw statements from within the code resulted in a TypeError with stack trace leading to Strapi parser middleware.
Instead, error logging should have appropriate stack trace from within the file where the error is thrown from.

How to test it?

Just in custom controller code, instead of usual ctx.throw(400, "something") write throw "something"

Reference: #9356 (comment)

Related issue(s)/PR(s)

Related issue: #9356

Additional notes?

No new test cases are provided. Even though this is a trivial change (and is just a safeguard from the runtime TypeError), I would be willing to provide new test cases but I would need some assistance of where those should be placed and how can this be thoroughly tested.
attn. @alexandrebodin @Convly

Throwing error inside of a controller (using plain javascript) results
in a different error in parser middleware. Before calling methods on the
`message` pr operty on the error object a check should be performed to
see if the `message` property is defined on the error instance.

✅ Closes: strapi#9356
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #9612 (006e362) into master (13d3ee8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9612   +/-   ##
=======================================
  Coverage   35.98%   35.98%           
=======================================
  Files        1337     1337           
  Lines       14768    14768           
  Branches     1475     1475           
=======================================
  Hits         5314     5314           
  Misses       8536     8536           
  Partials      918      918           
Flag Coverage Δ
front 27.36% <ø> (ø)
unit 55.54% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa301ab...00664a6. Read the comment docs.

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvement

@alexandrebodin alexandrebodin added source: core:strapi Source is core/strapi package issue: bug Issue reporting a bug labels Mar 5, 2021
@alexandrebodin alexandrebodin added this to the 3.5.3 milestone Mar 5, 2021
@alexandrebodin alexandrebodin merged commit 9fa64f3 into strapi:master Mar 5, 2021
This was referenced Mar 12, 2021
This was referenced Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
issue: bug Issue reporting a bug source: core:strapi Source is core/strapi package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants