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

volatile data can also be null #286

Merged
merged 2 commits into from
Mar 9, 2018
Merged

volatile data can also be null #286

merged 2 commits into from
Mar 9, 2018

Conversation

scottinet
Copy link
Contributor

When looking if a sdkInstance identifier is present in volatile data, we first check whether volatiles are defined or not. But they can also be null, leading to a sdk crash when that happens.
This PR fixes that issue.

@codecov-io
Copy link

codecov-io commented Mar 2, 2018

Codecov Report

Merging #286 into 6.x will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##              6.x     #286   +/-   ##
=======================================
  Coverage   80.27%   80.27%           
=======================================
  Files          19       19           
  Lines        2023     2023           
=======================================
  Hits         1624     1624           
  Misses        399      399
Impacted Files Coverage Δ
src/networkWrapper/protocols/abstract/realtime.js 52.38% <100%> (ø) ⬆️

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 7c54ee4...308b316. Read the comment docs.

@@ -212,7 +212,7 @@ class RTWrapper extends KuzzleEventEmitter {
return cb(error);
}
this.on(response.result.channel, data => {
data.fromSelf = data.volatile !== undefined && data.volatile.sdkInstanceId === this.id;
data.fromSelf = data.volatile !== undefined && data.volatile !== null && data.volatile.sdkInstanceId === this.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any chance data.volatile can be a scalar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, great

@@ -212,7 +212,7 @@ class RTWrapper extends KuzzleEventEmitter {
return cb(error);
}
this.on(response.result.channel, data => {
data.fromSelf = data.volatile !== undefined && data.volatile.sdkInstanceId === this.id;
data.fromSelf = data.volatile !== undefined && data.volatile !== null && data.volatile.sdkInstanceId === this.id;
Copy link
Contributor

Choose a reason for hiding this comment

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

ok, great

@benoitvidis benoitvidis merged commit 95e692b into 6.x Mar 9, 2018
ballinette pushed a commit that referenced this pull request Jun 5, 2018
* index controller

* server controller

* collection controller

* document controller

* wip

* wip

* wip - http

* rc - wo tests

*  fix vars scope

* tests - collection

* tests ms

* nitpicking

* re-add package-lock.json (previously removed by error by #286)

* ServerController: add jsdocs

* ServerController: add getStats method

* fix request.refresh argument

* inject kuzzle's volatile into requests

* remove defaultIndex

* remove obsolete unit tests on removed features

* unit-tests on security objects

* Refactor unit-tests for network protocols and query management

* refactor unit-tests for Kuzzle constructor, getters and setters

* fix kuzzle.connect unit tests

* fix unit tests for kuzzle eventEmitter/listener management

* fix unit tests for Kuzze netowrk methods

* fix unit tests for kuzzle query management

* fix linter

* fix unit-test for memoryStorage controller

* fix unit tests for auth controller

* remove obsolete mock

* fix unit test for bulk controller

* fix unit-test for collection controller

* improve naming for unit test groups

* unit-tests for server controller

* typo

* unit tests for document controller

* unit tests for index controller

* factorize security objects contructors

* nitpicking

* add unit tests for security controller

* add unit tests for DocumentSearchResult class

* add unit tests for SpecificationsSearchResult class

* add hits array to searchResult objects

* add unit tests for ProfileSearchResult class

* add unit tests for UserSearchResult class

* add unit tests for RoleSearchResult class

* add unit tests for RealTime controller and Room class

* harmonize error rejections (use throw instead of Promise.reject)
@scottinet scottinet deleted the volatile_can_be_null branch June 29, 2018 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants