Resolved issue calling method on null object #46

davidkirwan · 2018-05-01T09:53:45Z

JIRA: https://issues.jboss.org/browse/RHMAP-20186

What

Getting multiples of the following exception in the fh-mbaas logs on tom1-mbaas2-ship2:

FATAL: UncaughtException, please report: TypeError: Cannot read property 'error' of undefined
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/lib/impl/dataSources/updateCache.js:24:33
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/underscore/underscore.js:436:8
    at Function._.each._.forEach (/opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/underscore/underscore.js:145:9)
    at Function._.partition (/opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/underscore/underscore.js:435:7)
    at updateValidAndInvalidDataSources (/opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/lib/impl/dataSources/updateCache.js:23:24)
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/lib/impl/dataSources/updateCache.js:155:35
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/async/lib/async.js:229:13
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/async/lib/async.js:110:21
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/async/lib/async.js:24:16
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/async/lib/async.js:226:17
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/async/lib/async.js:482:30
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/lib/impl/dataSources/updateCache.js:232:18
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/mongoose/lib/model.js:3911:16
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/mongoose/lib/services/model/applyHooks.js:167:17
    at /opt/feedhenry/fh-mbaas/releases/5.10.4-5559b0e/node_modules/fh-forms/node_modules/fh-logger/node_modules/continuation-local-storage/node_modules/async-listener/glue.js:188:31
    at _combinedTickCallback (internal/process/next_tick.js:67:7)
    at process._tickCallback (internal/process/next_tick.js:98:9)

Why

fh-forms/lib/impl/dataSources/updateCache.js:24:33 contains the following:

var validInvalid = _.partition(dataSourcesToSplit, function(dataSourceUpdateData) {
    return !dataSourceUpdateData.error;
  });

I've not managed to figure out why yet, but it is possible for dataSourcesToSplit to be empty, or a particular element of the array to not have a error method. The fix will handle such situations.

How

The following snippet shows how the non existance of an object will no longer throw exception if checking is performed in this fashion:

> var f = function(test){ test.a; }
undefined
> f(undefined);
TypeError: Cannot read property 'a' of undefined
    at f (repl:1:29)
    at repl:1:1
    at REPLServer.defaultEval (repl.js:262:27)
    at bound (domain.js:287:14)
    at REPLServer.runBound [as eval] (domain.js:300:12)
    at REPLServer.<anonymous> (repl.js:431:12)
    at emitOne (events.js:82:20)
    at REPLServer.emit (events.js:169:7)
    at REPLServer.Interface._onLine (readline.js:211:10)
    at REPLServer.Interface._line (readline.js:550:8)
> var f2 = function(test){
... if(test){
..... return !test.a;
..... }
... return true;
... }
undefined
> f2(undefined);
true
> f2({a:false});
true
> f2({a:true});
false
>

coveralls · 2018-05-01T14:20:15Z

Coverage decreased (-0.01%) to 74.979% when pulling 4b1c3ab on davidkirwan:RHMAP-20186_fh-forms-updateCache-exception into 0a198f8 on feedhenry:master.

davidffrench · 2018-05-02T08:47:59Z

lib/impl/dataSources/updateCache.js

@@ -21,7 +21,11 @@ function updateValidAndInvalidDataSources(dataSourcesToSplit, currentInvalidData
  logger.debug("dataSourcesToSplit", dataSourcesToSplit);
  currentInvalidDataSources = currentInvalidDataSources || [];
  var validInvalid = _.partition(dataSourcesToSplit, function(dataSourceUpdateData) {
-    return !dataSourceUpdateData.error;
+    if (typeof dataSourceUpdateData.error === 'function') {


Hey @davidkirwan. I have a few questions. How do we know that dataSourceUpdateData.error should always be a function? and if it is not a function, the logic is now returning true and that there is no error. If this logic is true that it is a function, it then returns !dataSourceUpdateData.error which is not invoking the function it self.

davidffrench · 2018-05-02T08:48:59Z

test/accept/test_dataSource.js

@@ -720,8 +720,8 @@ module.exports.test = {
        forms.dataSources.updateCache(options, [dataSourceDataSingleChoice], {
          currentTime: new Date()
        }, function(err, badUpdateResult){
-          assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data");
-          assert.equal(ERROR_CODES.FH_FORMS_UNEXPECTED_ERROR, err.code, "Expected A FH_FORMS_UNEXPECTED_ERROR");
+          //assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data: " + util.inspect(err));


Were these assertions commented out by accident?

camilamacedo86 · 2018-05-02T08:54:18Z

Failling in the tests.

  164 passing (36s)
  1 failing
  1) test Test Update Data Source Data Set Wrong Format:
      Uncaught AssertionError: Expected 0 valid update
      + expected - actual
      -0
      +1
      
      at test/accept/test_dataSource.js:729:18
      at prepareResponse (lib/impl/dataSources/updateCache.js:9:16764)
      at fn (node_modules/async/lib/async.js:579:34)
      at Immediate.<anonymous> (node_modules/async/lib/async.js:495:34)
      at Immediate.<anonymous> (node_modules/fh-logger/node_modules/continuation-local-storage/node_modules/async-listener/glue.js:188:31)

davidkirwan · 2018-05-02T09:28:14Z

@camilamacedo86 @davidffrench Folks I'll add you as reviewers when I'm ready to have other eyes on the PR :) until then please ignore ! I cannot run these builds successfully locally, so having to rely on Jenkins/Travis builds to have the tests run etc for feedback.

davidffrench · 2018-05-02T09:43:23Z

ah sorry @davidkirwan :-)

camilamacedo86 · 2018-05-02T10:34:08Z

lib/impl/dataSources/updateCache.js

@@ -21,7 +21,11 @@ function updateValidAndInvalidDataSources(dataSourcesToSplit, currentInvalidData
  logger.debug("dataSourcesToSplit", dataSourcesToSplit);
  currentInvalidDataSources = currentInvalidDataSources || [];
  var validInvalid = _.partition(dataSourcesToSplit, function(dataSourceUpdateData) {
-    return !dataSourceUpdateData.error;


My suggestion to fix it follows. ( do just the following code )

if ( dataSourceUpdateData ){ return !dataSourceUpdateData.error; } return true;

PS. In JS/Node to check if the variable is != of undefined you can just if (var)

The original exceptions being raised are the following:

TypeError: Cannot read property 'error' of undefined

There seem to be situations where dataSourceUpdateData is being passed and is undefined, and calling .error on it raises an exception. So the suggested fix you make will not resolve this see the following as an example:

> var f = function(test){ test.a; } undefined > f(undefined); TypeError: Cannot read property 'a' of undefined at f (repl:1:29) at repl:1:1 at REPLServer.defaultEval (repl.js:262:27) at bound (domain.js:287:14) at REPLServer.runBound [as eval] (domain.js:300:12) at REPLServer.<anonymous> (repl.js:431:12) at emitOne (events.js:82:20) at REPLServer.emit (events.js:169:7) at REPLServer.Interface._onLine (readline.js:211:10) at REPLServer.Interface._line (readline.js:550:8) >

Now compare to the fix I'm proposing:

> var f2 = function(test) { ... if(typeof test === 'undefined'){ ..... return true; ..... } ... else{ ..... return test.a; ..... } ... } undefined > > > f2({a:1}); 1 > f2(undefined); true >

Opps my bad! I'm wrong here, this does appear to work!

> var f = function(test){ ... if(test){ ..... return !test.a; ..... } ... return true; ... } undefined > f(undefined); true > f({a:false}); true > f({a:true}); false >

davidkirwan · 2018-05-02T12:08:32Z

@camilamacedo86 @davidffrench the PR should now be in a position to be reviewed please!

camilamacedo86 · 2018-05-02T12:12:07Z

test/accept/test_dataSource.js

@@ -720,7 +720,7 @@ module.exports.test = {
        forms.dataSources.updateCache(options, [dataSourceDataSingleChoice], {
          currentTime: new Date()
        }, function(err, badUpdateResult){
-          assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data");
+          assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data: " + util.inspect(err));


IMHO you should not change the test.

This change is fine, as it aided in debugging that function. By adding the contents of the err object there.

davidffrench

Other than the one comment from Camila, looks good to me

camilamacedo86

Shows fine the change ( LGTB for me ).
For we are able to merge it:

Rebase with master
Bump version and CHANGELOG file

camilamacedo86 · 2018-05-02T13:15:44Z

test/accept/test_dataSource.js

@@ -720,7 +720,7 @@ module.exports.test = {
        forms.dataSources.updateCache(options, [dataSourceDataSingleChoice], {
          currentTime: new Date()
        }, function(err, badUpdateResult){
-          assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data");
+          assert.ok(err, "Expected An Error When Updating Data Sources With Bad Data: " + util.inspect(err));


davidkirwan · 2018-05-02T14:43:26Z

@camilamacedo86 seems since the rebase its got new changes you added in PR #45 can you take a look at the travis output perhaps you can determine why its failing based on your new changes?

camilamacedo86 · 2018-05-02T14:49:26Z

@davidkirwan all PR's before this one worked fine in the CI/Trevis. If it is not working shows that something broke in the rebase. I will give a look and let you know.

davidffrench · 2018-05-02T14:53:12Z

@camilamacedo86 @davidkirwan Travis can fail sometimes, always worth restarting the build when you think the build should have passed. I have restarted it now.

davidffrench · 2018-05-02T15:00:25Z

@davidkirwan Build has passed now

davidkirwan · 2018-05-02T15:00:50Z

Repeatable builds would be a nice feature to have eh. @davidffrench @camilamacedo86 if you are happy can you approve the PR. Thanks!

camilamacedo86 · 2018-05-02T15:02:23Z

@davidkirwan it still failing in the coverage/coveralls which means that you need to improve the tests in order to cover this scenario.

davidkirwan · 2018-05-02T15:03:11Z

@camilamacedo86 feel free to suggest an improvement here.

camilamacedo86 · 2018-05-02T15:13:04Z

@davidkirwan the coverage check is automatic. To check it see the following:

Click on the datails for further information.

For you be allowed to merge it the PR need pass in all automatic checks + the code review ( some approval )

I'd like to recommend you add a test here which covers the scenario which was fixed in the PR.

PS.: I approved the PR but please wait for the @davidffrench confirmation too.
@davidffrench could we merge it without adding more tests here? ( the coverage/coveralls — Coverage decreased (-0.01%) to 74.979% is falling because it is -0,01% , however for this case I think it is fine and more tests will be unnecessary )

camilamacedo86 · 2018-05-04T09:23:15Z

@davidkirwan merged

davidkirwan · 2018-05-04T10:33:55Z

@camilamacedo86 @davidffrench @grdryn cheers folks!

davidffrench reviewed May 2, 2018

View reviewed changes

camilamacedo86 reviewed May 2, 2018

View reviewed changes

camilamacedo86 suggested changes May 2, 2018

View reviewed changes

davidffrench reviewed May 2, 2018

View reviewed changes

camilamacedo86 reviewed May 2, 2018

View reviewed changes

camilamacedo86 approved these changes May 2, 2018

View reviewed changes

Resolved issue calling method on null object

4b1c3ab

davidffrench approved these changes May 4, 2018

View reviewed changes

camilamacedo86 merged commit cbd257a into feedhenry:master May 4, 2018

davidkirwan deleted the RHMAP-20186_fh-forms-updateCache-exception branch May 4, 2018 10:33

davidkirwan mentioned this pull request May 4, 2018

Updated fh-forms from 1.16.6 -> 1.16.7 feedhenry/fh-mbaas#111

Merged

This was referenced May 4, 2018

[fix] - regards the PR #46 #47

Closed

Revert "Resolved issue calling method on null object" #48

Closed

davidffrench mentioned this pull request May 16, 2018

Rhmap 20186 cp feedhenry/fh-mbaas#113

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolved issue calling method on null object #46

Resolved issue calling method on null object #46

davidkirwan commented May 1, 2018 •

edited

coveralls commented May 1, 2018 •

edited

davidffrench May 2, 2018

davidffrench May 2, 2018

camilamacedo86 commented May 2, 2018

davidkirwan commented May 2, 2018

davidffrench commented May 2, 2018

camilamacedo86 May 2, 2018 •

edited

davidkirwan May 2, 2018 •

edited

davidkirwan May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 May 2, 2018

davidkirwan May 2, 2018

camilamacedo86 May 2, 2018

davidffrench left a comment

camilamacedo86 left a comment

camilamacedo86 May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018

davidffrench commented May 2, 2018

davidffrench commented May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018 •

edited

camilamacedo86 commented May 4, 2018

davidkirwan commented May 4, 2018

Resolved issue calling method on null object #46

Resolved issue calling method on null object #46

Conversation

davidkirwan commented May 1, 2018 • edited

What

Why

How

coveralls commented May 1, 2018 • edited

davidffrench May 2, 2018

Choose a reason for hiding this comment

davidffrench May 2, 2018

Choose a reason for hiding this comment

camilamacedo86 commented May 2, 2018

davidkirwan commented May 2, 2018

davidffrench commented May 2, 2018

camilamacedo86 May 2, 2018 • edited

Choose a reason for hiding this comment

davidkirwan May 2, 2018 • edited

Choose a reason for hiding this comment

davidkirwan May 2, 2018

Choose a reason for hiding this comment

davidkirwan commented May 2, 2018

camilamacedo86 May 2, 2018

Choose a reason for hiding this comment

davidkirwan May 2, 2018

Choose a reason for hiding this comment

camilamacedo86 May 2, 2018

Choose a reason for hiding this comment

davidffrench left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 May 2, 2018

Choose a reason for hiding this comment

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018

davidffrench commented May 2, 2018

davidffrench commented May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018

davidkirwan commented May 2, 2018

camilamacedo86 commented May 2, 2018 • edited

camilamacedo86 commented May 4, 2018

davidkirwan commented May 4, 2018

davidkirwan commented May 1, 2018 •

edited

coveralls commented May 1, 2018 •

edited

camilamacedo86 May 2, 2018 •

edited

davidkirwan May 2, 2018 •

edited

camilamacedo86 commented May 2, 2018 •

edited