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.

Sign up for GitHub

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

Fixed falsey/null value issues in jsong merge #124

Merged
merged 1 commit into from
Aug 25, 2015
Merged

Fixed falsey/null value issues in jsong merge #124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/cache/jsongMerge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,13 +44,14 @@ function merge(config, cache, message, depth, path, fromParent, fromKey) {
}

// The message at this point should always be defined.
if (message.$type || typeOfMessage !== 'object') {
// Reached the end of the JSONG message path
if (message === null || typeOfMessage !== 'object' || message.$type) {
fromParent[fromKey] = clone(message);

// NOTE: If we have found a reference at our cloning position
// and we have resolved our path then add the reference to
// the unfulfilledRefernces.
if (message.$type === $ref) {
if (message && message.$type === $ref) {
var references = config.references;
references.push({
path: cloneArray(requestedPath),
Expand All @@ -65,7 +66,7 @@ function merge(config, cache, message, depth, path, fromParent, fromKey) {
var values = config.values;
values.push({
path: cloneArray(requestedPath),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelbpaulson, @jhusain

The message.type here seems to be a bug also, but 'fixing' it (I assume it's supposed to be message.$type as opposed to message.type) breaks the JSONG - Merge should write a simple path to the cache basic unit test in jsongMerge.spec.js. Needed some input on what the expected output should be for this test to determine if the test is broken, or there are potentially more bugs in the jsongMerge impl, which need to be teased out.

This is the failing test, if I change this line to be message.$type:

1) Router Unit Internals JSONG - Merge should write a simple path to the cache.:

      AssertionError: expected { Object (references, values) } to deeply equal { Object (values, references) }
      + expected - actual

             "path": [
               "there"
               "is"
             ]
      -      "value": "a value"
      +      "value": {
      +        "$type": "atom"
      +        "value": "a value"
      +      }
           }
         ]
       }

Copy link
Contributor

Choose a reason for hiding this comment

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

@sdesai you are correct. I think the good news is that we remove that statement and only ever use the value that is handed to us. This could potentially strip some valuable meta data on a node by using the .value of the handed back value.

So what I am saying is that you were correct in assuming that this is incorrect, because it is. But the fix is to remove message.type ? message.value : message with message

value: message.type ? message.value : message
value: (message && message.type) ? message.value : message
});
}

Expand All @@ -85,20 +86,21 @@ function merge(config, cache, message, depth, path, fromParent, fromKey) {
// just follow cache, else attempt to follow message.
var cacheRes = cache[key];
var messageRes = message[key];

var nextPath = path;
var nextDepth = depth + 1;
if (updateRequestedPath) {
requestedPath[requestIdx] = key;
}

// Cache does not exist but message does.
if (!cacheRes) {
if (cacheRes === undefined) {
cacheRes = cache[key] = {};
}

// TODO: Can we hit a leaf node in the cache when traversing?

if (messageRes) {
if (messageRes !== undefined) {
var nextIgnoreCount = 0;

// TODO: Potential performance gain since we know that
Expand All @@ -107,7 +109,7 @@ function merge(config, cache, message, depth, path, fromParent, fromKey) {

// There is only a need to consider message references since the
// merge is only for the path that is provided.
if (messageRes.$type === $ref && depth < path.length - 1) {
if (messageRes && messageRes.$type === $ref && depth < path.length - 1) {
nextDepth = 0;
nextPath = catAndSlice(messageRes.value, path, depth + 1);
cache[key] = clone(messageRes);
Expand Down
133 changes: 133 additions & 0 deletions test/unit/core/get.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ var chai = require('chai');
var expect = chai.expect;
var falcor = require('falcor');
var $ref = falcor.Model.ref;
var $atom = falcor.Model.atom;
var Observable = require('rx').Observable;
var sinon = require('sinon');

Expand All @@ -25,6 +26,138 @@ describe('Get', function() {
});
});

it('should not return empty atoms for a null value in jsonGraph', function(done) {

var router = new R([{
route: 'videos.falsey',
get: function(path) {
return Observable.return({
jsonGraph: {
videos: {
falsey: null
}
},
paths: [['videos', 'falsey']]
});
}
}]);

var onNext = sinon.spy();

router.get([['videos', 'falsey']]).
doAction(onNext).
doAction(noOp, noOp, function() {
expect(onNext.calledOnce).to.be.ok;
expect(onNext.getCall(0).args[0]).to.deep.equals({
jsonGraph: {
videos: {
falsey: null
}
}
});
}).
subscribe(noOp, done, done);
});

it('should not return empty atoms for a null value atom in jsonGraph', function(done) {

var router = new R([{
route: 'videos.falsey',
get: function(path) {
return Observable.return({
jsonGraph: {
videos: {
falsey: $atom(null)
}
},
paths: [['videos', 'falsey']]
});
}
}]);

var onNext = sinon.spy();

router.get([['videos', 'falsey']]).
doAction(onNext).
doAction(noOp, noOp, function() {
expect(onNext.calledOnce).to.be.ok;
expect(onNext.getCall(0).args[0]).to.deep.equals({
jsonGraph: {
videos: {
falsey: $atom(null)
}
}
});
}).
subscribe(noOp, done, done);
});

it('should not return empty atoms for a zero value in jsonGraph', function(done) {

var router = new R([{
route: 'videos.falsey',
get: function(path) {
return Observable.return({
jsonGraph: {
videos: {
falsey: 0
}
},
paths: [['videos', 'falsey']]
});
}
}]);

var onNext = sinon.spy();

router.get([['videos', 'falsey']]).
doAction(onNext).
doAction(noOp, noOp, function() {
expect(onNext.calledOnce).to.be.ok;
expect(onNext.getCall(0).args[0]).to.deep.equals({
jsonGraph: {
videos: {
falsey: 0
}
}
});
}).
subscribe(noOp, done, done);
});

it('should not return empty atoms for a zero value atom in jsonGraph', function(done) {

var router = new R([{
route: 'videos.falsey',
get: function(path) {
return Observable.return({
jsonGraph: {
videos: {
falsey: $atom(0)
}
},
paths: [['videos', 'falsey']]
});
}
}]);

var onNext = sinon.spy();

router.get([['videos', 'falsey']]).
doAction(onNext).
doAction(noOp, noOp, function() {
expect(onNext.calledOnce).to.be.ok;
expect(onNext.getCall(0).args[0]).to.deep.equals({
jsonGraph: {
videos: {
falsey: $atom(0)
}
}
});
}).
subscribe(noOp, done, done);
});

it('should not return empty atoms for a zero path value', function(done) {

var router = new R([{
Expand Down
89 changes: 89 additions & 0 deletions test/unit/internal/jsongMerge.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var _ = require('lodash');
* are.
*/
describe('JSONG - Merge', function() {

it('should write a simple path to the cache.', function() {

var jsong = {
Expand All @@ -33,6 +34,91 @@ describe('JSONG - Merge', function() {
references: []
});
});

it('should write a falsey number to the cache.', function() {

var jsong = {
jsonGraph: {
there: {
is: 0
}
},
paths: [['there', 'is']]
};

var out = mergeTest(jsong);
expect(out).to.deep.equals({
values: [{
path: ['there', 'is'],
value: 0
}],
references: []
});
});

it('should write a falsey string to the cache.', function() {

var jsong = {
jsonGraph: {
there: {
is: ''
}
},
paths: [['there', 'is']]
};

var out = mergeTest(jsong);
expect(out).to.deep.equals({
values: [{
path: ['there', 'is'],
value: ''
}],
references: []
});
});

it('should write a false boolean to the cache.', function() {

var jsong = {
jsonGraph: {
there: {
is: false
}
},
paths: [['there', 'is']]
};

var out = mergeTest(jsong);
expect(out).to.deep.equals({
values: [{
path: ['there', 'is'],
value: false
}],
references: []
});
});

it('should write a null to the cache.', function() {

var jsong = {
jsonGraph: {
there: {
is: null
}
},
paths: [['there', 'is']]
};

var out = mergeTest(jsong);
expect(out).to.deep.equals({
values: [{
path: ['there', 'is'],
value: null
}],
references: []
});
});

it('should write a path with a reference to a value.', function() {
var jsong = {
jsonGraph: {
Expand All @@ -47,6 +133,7 @@ describe('JSONG - Merge', function() {
};
mergeTest(jsong);
});

it('should write a path with a reference to a branch.', function() {

var jsong = {
Expand All @@ -63,6 +150,7 @@ describe('JSONG - Merge', function() {

mergeTest(jsong);
});

it('should write a path with a reference to a reference.', function() {
var jsong = {
jsonGraph: {
Expand All @@ -77,6 +165,7 @@ describe('JSONG - Merge', function() {

mergeTest(jsong);
});

it('should get the set refs.', function() {
var jsong = {
jsonGraph: {
Expand Down