Fixed apiType and broken error handling
#10536
Conversation
no issue
- throwing an object from a catch handler is not a good idea
- unexpected and broke functional call to API (always returned a 500, because API returned {err: err, method: ...}
no issue - consistency
no issue - was unable to revert TryGhost@9dd7aff, because it contains members changes - functional calls did not work correctly, because the content and admin ctrl differentiation happend in the web layer - `isContentAPI` returned true for `api.v2.settings.edit(data, {context: {internal:true{})` - content & admin API are using different controllers - we can just tell which ctrl is content API and which is not - the direction fits for the content & admin API split
kirrg001
force-pushed
the
fix-api-type
branch
from
3bd3321 to
9bc9945
Compare
kirrg001
force-pushed
the
fix-api-type
branch
from
9bc9945 to
4d3f6a3
Compare
| @@ -156,9 +159,6 @@ const pipeline = (apiController, apiUtils) => { | |||
| }) | |||
| .then(() => { | |||
| return frame.response; | |||
| }) | |||
| .catch((err) => { | |||
| throw {err, docName, method}; | |||
There was a problem hiding this comment.
@gargol Which consequences has this change on the error message right now? e.g. the site uses browse & read only, which most common error is a 404.
There was a problem hiding this comment.
As long as frameOptions are populated with docName and method that map to these rules: https://github.com/TryGhost/Ghost/blob/master/core/server/web/shared/middlewares/error-handler.js#L96-L106 there should be no effect. The 404 for read/browse would map to read/list {resource} and this translation:
"NotFoundError": "Resource not found error, cannot {action}.",
| @@ -103,10 +103,6 @@ module.exports = { | |||
| return shared.pipeline(require('./slack'), localUtils); | |||
There was a problem hiding this comment.
Every content API ctrl uses the pattern {resource}-public, except of this ctrl. Consistency change.
| @@ -10,7 +10,7 @@ class Frame { | |||
| this.user = {}; | |||
There was a problem hiding this comment.
@allouis Please review & test the apiType change good as possible 👍 Thanks!
| return shared.pipeline(require('./tags-public'), localUtils, 'content'); | ||
| }, | ||
|
|
||
| get publicSettings() { |
There was a problem hiding this comment.
Needs renaming, not important now.
There was a problem hiding this comment.
Would be best tackled during #10106, right?
|
|
||
| // CASE: apiType = 'content' for HTTP Content API | ||
| return frame.apiType === 'content' || isPublic; | ||
| return frame.apiType === 'content'; |
There was a problem hiding this comment.
Doesn't matter which context you have. If you request content API, you are in content ctrl land. Same for admin.
|
This looks good - I will test A$AP |
|
Have tested the part which had to do with changes in error handling, works great and is a lot cleaner than the previous implementation 👍 |
|
@allouis Merged. Can you please confirm back if you have tested the apiType change? Thanks! |
PR contains a couple of clean commits. Please look at each commit individually 👍
A couple of regression tests were broken, which made me curious.
API v2 pipeline changes the error handler, which broke the error handling for the site, because it expects an error and not an object with attrs. If you add a catch handler, you expect to get an error and not an object.
apiTypewas always undefined for functional calls e.g.api.v2.settings.edit({..} {context: {internal:true}})returned true forisContentAPI(). TheapiTypewas only handled on http layer, not on functional layer.Please use "rebase & commit"!