Allow safeFields to work with arrays #73

shimks · 2018-05-31T17:22:22Z

Description

Previously when an array of errors are encountered, full information on the errors was not revealed to the response unless debug mode was specifically enabled; even if safeFields was set the properties in it would not show in the response. This PR fixes this.

fixes Safefields doesn't work with "array of errors" #45

Related issues

connect to <link_to_referenced_issue>

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

bajtos

IIUC, this change will always include all error properties in the response. That would be a security vulnerability!

Please rework the patch to ensure that:

in debug mode, all error properties are sent in the response
in production mode, only error properties listed in safeFields are sent in the response

I think the existing test is mostly covering the first debug-mode case, you will need to add another test to cover the production mode.

bajtos · 2018-06-04T08:12:36Z

Please note that safeFields are referring to fields of the Error objects (array items) received by strong-error-handler, not to the properties of the synthetic error created by our error handler.

shimks · 2018-06-04T15:37:39Z

From what I'm understanding, safeFields should cover the fields in the items of the array, right? In that case, I wonder if we should expose name and message properties when safeFields is set?

{statusCode: 500, name: 'ArrayOfErrors, message: 'Failed with multiple errors...', details: ...}
// vs
{statusCode: 500, details: ...}

I guess it's rather a minute detail

bajtos · 2018-06-05T07:57:30Z

I wonder if we should expose name and message properties

This is a good question! Since we want to include save details about errors in the array, I feel it's better to signal to the consumers that the error response is of type ArrayOfErrors.

bajtos · 2018-06-05T08:53:36Z

@shimks I'd like to propose an alternative implementation that leverages buildResponseData algorithm for building details of individual error items. As a result, the format of details items will be the same as if each error was send back to the client individually. I think this is more consistent and thus better for users.

Rather than trying to explain it, I figured out it will be faster to implement the changes myself. I added a new commit to your feature branch - see 44fc5fc. Please let me know what do you think about such proposal.

bajtos · 2018-06-05T08:55:28Z

test/handler.test.js

+          message: 'The model instance is not valid.',
+          name: 'ValidationError',
+          code: 'VALIDATION_ERROR',
+          details: 'some details',


Please notice that for 4xx errors, we always include additional fields beyond safeFields. If we are going to list individual errors in production (debug=false) mode, then I think we should apply the same rules.

bajtos · 2018-06-05T08:58:04Z

test/handler.test.js

+          {statusCode: 500, message: 'Internal Server Error'},
+          {statusCode: 500, message: 'Internal Server Error'},
+          {statusCode: 500, message: 'Internal Server Error'},
+        ]);


This is a bit ugly. Before, we would return a short error response:

{ error: { statusCode: 500, message: 'Internal Server Error', }, });

Now we are returning more details, but these details are not really helpful:

{ error: { statusCode: 500, message: 'Internal Server Error', details: [ {statusCode: 500, message: 'Internal Server Error'}, {statusCode: 500, message: 'Internal Server Error'}, {statusCode: 500, message: 'Internal Server Error'}, ], } }

It may be possible to implement some heuristics to omit these "generic" entries from the output, but don't think it's worth the effort and additional complexity.

shimks

Your new commit looks good to me. I can merge if you're ok with the commit as well.

A question on the side, are we able to use const with lb3 related packages? I recall there being talks of dropping node 4 but I don't remember what came of it.

bajtos · 2018-06-05T14:53:28Z

A question on the side, are we able to use const with lb3 related packages? I recall there being talks of dropping node 4 but I don't remember what came of it.

Yes, we should use const everywhere in LB 3.x codebase - see http://loopback.io/doc/en/contrib/style-guide-es6.html. It's LB 2.x with its support for Node.js 0.10 and 0.12 where we should not use const - see http://loopback.io/doc/en/contrib/style-guide-es5.html

Your new commit looks good to me. I can merge if you're ok with the commit as well.

Let's do it! Please squash the commits and preserve information about my (co)authorship.

shimks · 2018-06-05T20:31:47Z

@slnode test please

shimks · 2018-06-06T15:23:07Z

The downstream test for strong-remoting seems to be failing because we now provide errorCode on each items of the array when it used to not. I think we can land this PR and follow up with a subsequent PR on strong-remoting to fix up the assertions a bit. Thoughts @bajtos?

bajtos · 2018-06-07T08:46:40Z

The downstream test for strong-remoting seems to be failing because we now provide errorCode on each items of the array when it used to not. I think we can land this PR and follow up with a subsequent PR on strong-remoting to fix up the assertions a bit. Thoughts @bajtos?

Sure. Just make sure to create the subsequent PR soon, so that we don't have strong-remoting tests failing for too long.

bajtos · 2018-06-07T08:52:56Z

@shimks I think we should release this change as semver-major, to ensure we don't accidentally break somebody's clients expecting the old format. Thoughts?

If we go down that route, then we should also:

Before the release:
- Drop support for Node.js 4.x as part of that release (if it haven't been done already)
- Check if there are any dependencies that we were not able to upgrade in semver-minor release because of their breaking changes - these need to be upgraded as part of the release too
- Update README with information about current/LTS/maintenance versions, see API Explorer for an example
After the release:
Update places like loopback-workspace (both package.json and templates), loopback-sanbox to use the new major version of strong-error-handler.
The same applies to other dependents - loopback dev-dependencies, strong-remoting dev-dependencies, etc.

The benefit is clear - we won't break any CI builds, because the fix of strong-remoting tests will go hand-in-hand with the upgrade to a newer strong-error-handler version.

shimks · 2018-06-07T14:24:49Z

That sounds good to me. I can open the follow up PRs for dropping node 4 (if not done already) and updating to the latest dependencies after I land this one.

b-admike

💯

Co-authored-by: Miroslav Bajtos <mbajtoss@gmail.com>

shimks requested review from bajtos and zbarbuto as code owners May 31, 2018 17:22

virkt25 approved these changes May 31, 2018

View reviewed changes

b-admike approved these changes May 31, 2018

View reviewed changes

zbarbuto approved these changes May 31, 2018

View reviewed changes

bajtos suggested changes Jun 4, 2018

View reviewed changes

bajtos force-pushed the array-safefields branch from f7dc0cc to 44fc5fc Compare June 5, 2018 08:52

bajtos reviewed Jun 5, 2018

View reviewed changes

shimks commented Jun 5, 2018

View reviewed changes

bajtos assigned shimks Jun 5, 2018

shimks force-pushed the array-safefields branch from 44fc5fc to 89b25fc Compare June 5, 2018 16:55

bajtos approved these changes Jun 7, 2018

View reviewed changes

shimks mentioned this pull request Jun 7, 2018

Drop node 4, update dependencies and status on maintenance #74

Merged

2 tasks

b-admike approved these changes Jun 8, 2018

View reviewed changes

Allow safeFields to work with arrays

7828534

Co-authored-by: Miroslav Bajtos <mbajtoss@gmail.com>

shimks force-pushed the array-safefields branch from 89b25fc to 7828534 Compare June 8, 2018 19:02

shimks merged commit 0a39f84 into master Jun 11, 2018

shimks deleted the array-safefields branch June 11, 2018 15:11

shimks mentioned this pull request Jun 11, 2018

New release #75

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow safeFields to work with arrays #73

Allow safeFields to work with arrays #73

shimks commented May 31, 2018

bajtos left a comment

bajtos commented Jun 4, 2018

shimks commented Jun 4, 2018 •

edited

bajtos commented Jun 5, 2018

bajtos commented Jun 5, 2018

bajtos Jun 5, 2018

bajtos Jun 5, 2018

shimks Jun 5, 2018

shimks left a comment

bajtos commented Jun 5, 2018

shimks commented Jun 5, 2018

shimks commented Jun 6, 2018

bajtos commented Jun 7, 2018

bajtos commented Jun 7, 2018

shimks commented Jun 7, 2018

b-admike left a comment

Allow safeFields to work with arrays #73

Allow safeFields to work with arrays #73

Conversation

shimks commented May 31, 2018

Description

Related issues

Checklist

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Jun 4, 2018

shimks commented Jun 4, 2018 • edited

bajtos commented Jun 5, 2018

bajtos commented Jun 5, 2018

bajtos Jun 5, 2018

Choose a reason for hiding this comment

bajtos Jun 5, 2018

Choose a reason for hiding this comment

shimks Jun 5, 2018

Choose a reason for hiding this comment

shimks left a comment

Choose a reason for hiding this comment

bajtos commented Jun 5, 2018

shimks commented Jun 5, 2018

shimks commented Jun 6, 2018

bajtos commented Jun 7, 2018

bajtos commented Jun 7, 2018

shimks commented Jun 7, 2018

b-admike left a comment

Choose a reason for hiding this comment

shimks commented Jun 4, 2018 •

edited