Error codes normalization #1481
Conversation
Data was written to the payload assuming 1 char = 1 byte, but with non-utf8 char we need to use the byteLength property from the buffer and not just the string length.
Update cluster version to 3.2.12
| 'services', | ||
| 'storage', | ||
| 'incomplete_delete', | ||
| errors)); | ||
| } | ||
|
|
||
| // @todo should be done in clientAdapter ? |
There was a problem hiding this comment.
| @@ -1438,9 +1436,7 @@ class ElasticSearch extends Service { | |||
|
|
|||
| // @todo should be done in clientAdapter | |||
There was a problem hiding this comment.
Codecov Report
@@ Coverage Diff @@
## 2-dev #1481 +/- ##
==========================================
+ Coverage 92.63% 92.64% +0.01%
==========================================
Files 98 98
Lines 6582 6637 +55
==========================================
+ Hits 6097 6149 +52
- Misses 485 488 +3
Continue to review full report at Codecov.
|
| const | ||
| flagName = flagPath.split('.').slice(-1), | ||
| flagValue = _.get(request, `input.${flagPath}`); | ||
| const flagValue = _.get(request, `input.${flagPath}`); |
There was a problem hiding this comment.
I think we stop using path in this method too. We only use this method to extract notify and includeKuzzleMeta from input.args
There was a problem hiding this comment.
This review seems to have been made on an obsolete version of that branch: the method tryGetBoolean was removed in the optimization commit
| assertionError.throw('missing_argument', errorName); | ||
| } | ||
|
|
||
| value = Number.parseFloat(value); |
There was a problem hiding this comment.
parseInt truncates numbers to transform them into integers:
> Number.parseInt('3.14')
3
We don't want to change provided values silently, so we need to parse the number as a floating-point value, and then verify that it's an integer number.
| getBodyNumber (request, name, def = null) { | ||
| const body = request.input.body; | ||
|
|
||
| if (body === null) { |
There was a problem hiding this comment.
Why not use getBody method here and in the other methods ?
There was a problem hiding this comment.
For the same reason we made separate methods for these getters: to use getBody, we'd need to use it without a default value, otherwise it would return different types, triggering a de-opt, cascaded to callers.
So, we have a choice between (current):
const body = request.input.body;
if (body === null) {
if (def !== null) {
return def;
}
assertionError.throw('body_required');
}And:
let body;
try {
body = getBody(request);
}
catch (e) {
if (def !== null) {
return def;
}
throw e;
}Which is longer and more complicated.
Co-Authored-By: Adrien Maret <amaret93@gmail.com>
|
SonarQube analysis reported 11 issues
Watch the comments in this conversation to review them. 9 extra issuesNote: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:
|
# [2.0.0-rc.2](https://github.com/kuzzleio/kuzzle/releases/tag/2.0.0-rc.2) (2019-11-05) #### Breaking changes - [ [#1500](#1500) ] Remove Dsl constructor from plugin context ([Aschen](https://github.com/Aschen)) - [ [#1489](#1489) ] Drop support for socket.io ([scottinet](https://github.com/scottinet)) - [ [#1491](#1491) ] Remove strategy constructors support ([Aschen](https://github.com/Aschen)) - [ [#1476](#1476) ] Change Document controller m* routes returns & fix SDK functional tests ([Aschen](https://github.com/Aschen)) #### Bug fixes - [ [#1512](#1512) ] Fix subscribing with a dead connection ([benoitvidis](https://github.com/benoitvidis)) - [ [#1507](#1507) ] Properly destroy a user after a credentials creation failure ([scottinet](https://github.com/scottinet)) - [ [#1483](#1483) ] Fix socketio connections leak & deactivate this protocol by default ([scottinet](https://github.com/scottinet)) #### New features - [ [#1502](#1502) ] Add a new "document:exists" API route ([scottinet](https://github.com/scottinet)) - [ [#1501](#1501) ] Add collection delete ([Aschen](https://github.com/Aschen)) - [ [#1499](#1499) ] Expose an Elasticsearch client constructor in plugin context ([Aschen](https://github.com/Aschen)) - [ [#1484](#1484) ] Upgrade script ([scottinet](https://github.com/scottinet)) #### Enhancements - [ [#1510](#1510) ] Mapping - enable dynamic templates ([benoitvidis](https://github.com/benoitvidis)) - [ [#1495](#1495) ] Check for SDK compatibility against current Kuzzle version ([Aschen](https://github.com/Aschen)) - [ [#1498](#1498) ] Add highlight to search results ([Aschen](https://github.com/Aschen)) - [ [#1488](#1488) ] Add kuzzle version to the dumped informations ([scottinet](https://github.com/scottinet)) - [ [#1481](#1481) ] Error codes normalization ([scottinet](https://github.com/scottinet)) - [ [#1478](#1478) ] Error codes redux ([scottinet](https://github.com/scottinet)) ---
Can be merged, but do not release Kuzzle until kuzzleio/kuzzle-common-objects#84 is also merged, and this repository's dependencies updated accordingly
Description
Errors normalization is back.
Same as for the v1 (#1478 ), but updated for Kuzzle v2.
For a complete list of changes about error codes, check the other PR. Here I'll detail the changes specific to Kuzzle v2:
baseController.tryGetBooleanand thebaseController.getXxxParammethods, because error messages thrown couldn't be protocol agnostic (this could be confusing for users, especially for those using HTTP). Also, the previous methods used property paths, which is slower than accessing directly to a property (until now, we don't need paths). The new methods now are:getBody<Type<(request, name, default): gets a body property. Handled types:object, array, string, number, integer, boolean("integer" is new and enforce an integer number)getArg(request, name, default): gets an optional property (in request.input.args). Handled types:object, array, string, number, integer, boolean("integer" is new and enforce an integer number)getBody(request, default): throws a dedicatedapi.assert.body_requirederror if the request doesn't have a body, unless a default value is providedHow to review
Since #1478 has been merged, this review should only be about the changes made in the BaseController class, and the updated v2 documentation.