Return 500 when DB is not connected (fix for issue 570)

[Gauravp-NEC]

This PR fixes the issue : #570

If database is not connected and any API will be requested then it will return "500 internal server error" and in logs it will display "database is not connected"

[Gauravp-NEC]

@AlvaroVega , Please review the PR. Thanks :)

[fgalan]

I'm afraid this PR cannot be merged in its current form. Test are not passing.

In addition, an entry about the fixes in this PR should be added to CHANGES_NEXT_RELEASE file.

[AlvaroVega]

@Gauravp-NEC could you please add any kind of tests which uses this PR? This way we can check and understand better what case is not covered by mongo driver reconnect mechanism.

[fgalan]

I'm afraid this PR cannot be merged in its current form. Test are not passing.

We have been seeing similar fails in other Node-based repositories (see thread: telefonicaid/iotagent-node-lib#1119 (comment)). Let's wait to see if it somekind of overall Github Actions issue...

[Gauravp-NEC]

I'm afraid this PR cannot be merged in its current form. Test are not passing.

We have been seeing similar fails in other Node-based repositories (see thread: telefonicaid/iotagent-node-lib#1119 (comment)). Let's wait to see if it somekind of overall Github Actions issue...

@fgalan , If the test case failure is overall GitHub action issue, this means that test case failure is not because of this PR. Please merge the PR. I have also tested this PR in my development environment and it is working fine.

[Gauravp-NEC]

@Gauravp-NEC could you please add any kind of tests which uses this PR? This way we can check and understand better what case is not covered by mongo driver reconnect mechanism.

@AlvaroVega , Done in commit 1e3615c

[fgalan]

@fgalan , If the test case failure is overall GitHub action issue, this means that test case failure is not because of this PR. Please merge the PR. I have also tested this PR in my development environment and it is working fine.

Note that although tests get working in your local environment, we also need to ensure that they pass at Github Actions. Github Actions testing is a "common ground" to run regression, so we can avoid any potential "byassing" in local environments.

In this case, the issue was solved in master branch, your PR synched with master and now the tests are passing. This is the way to go :)

[AlvaroVega]

LGTM

[mapedraza]

@Gauravp-NEC I just copied your code to a new branch and modify the test in this PR #577

The test implemented queries the API (GET /STH/v2/entities/entityId/attrs/attrName?type=entityType&lastN=1) with the server started and the DB connection previously closed. The test implemented is the following:

         describe('When the database is not connected', function() {
            before(function(done) {
                sth.sthServer.startServer(sthConfig.STH_HOST, sthConfig.STH_PORT, function(err, server) {
                    sthDatabase.closeConnection(function() {
                        done();
                    });
                });
            });
            it('should return 500', function(done) {
                let url_sth =
                    'http://' +
                    sthConfig.STH_HOST +
                    ':' +
                    sthConfig.STH_PORT +
                    '/STH/v2/entities/entityId/attrs/attrName?type=entityType&lastN=1';
                request(
                    {
                        uri: url_sth,
                        method: 'GET',
                        headers: {
                            'Fiware-Service': sthConfig.DEFAULT_SERVICE,
                            'Fiware-ServicePath': sthConfig.DEFAULT_SERVICE_PATH
                        }
                    },
                    function(err, response, body) {
                        expect(response.statusCode).to.equal(500);
                        done();
                    }
                );
            });

            after(function(done) {
                sth.exitGracefully(null, done);
            });
        });
    });

The following error is given when requesting the API without the database connected:

(node:30203) [MONGODB DRIVER] Warning: the options [servers] is not supported
(node:30203) [MONGODB DRIVER] Warning: the options [caseTranslate] is not supported
(node:30203) [MONGODB DRIVER] Warning: the options [dbName] is not supported
        1) should return 500

This seems the purpose of the PR is not satisfied since there is no an answer when requesting without the DB connected

[Gauravp-NEC]

@Gauravp-NEC I just copied your code to a new branch and modify the test in this PR #577

The test implemented queries the API (GET /STH/v2/entities/entityId/attrs/attrName?type=entityType&lastN=1) with the server started and the DB connection previously closed. The test implemented is the following:

         describe('When the database is not connected', function() {
            before(function(done) {
                sth.sthServer.startServer(sthConfig.STH_HOST, sthConfig.STH_PORT, function(err, server) {
                    sthDatabase.closeConnection(function() {
                        done();
                    });
                });
            });
            it('should return 500', function(done) {
                let url_sth =
                    'http://' +
                    sthConfig.STH_HOST +
                    ':' +
                    sthConfig.STH_PORT +
                    '/STH/v2/entities/entityId/attrs/attrName?type=entityType&lastN=1';
                request(
                    {
                        uri: url_sth,
                        method: 'GET',
                        headers: {
                            'Fiware-Service': sthConfig.DEFAULT_SERVICE,
                            'Fiware-ServicePath': sthConfig.DEFAULT_SERVICE_PATH
                        }
                    },
                    function(err, response, body) {
                        expect(response.statusCode).to.equal(500);
                        done();
                    }
                );
            });

            after(function(done) {
                sth.exitGracefully(null, done);
            });
        });
    });

The following error is given when requesting the API without the database connected:

(node:30203) [MONGODB DRIVER] Warning: the options [servers] is not supported
(node:30203) [MONGODB DRIVER] Warning: the options [caseTranslate] is not supported
(node:30203) [MONGODB DRIVER] Warning: the options [dbName] is not supported
        1) should return 500

This seems the purpose of the PR is not satisfied since there is no an answer when requesting without the DB connected

@mapedraza @fgalan , In this PR currently only the test case part is might not satisfying.I would have fixed the test case after the review comment. Rest the code and logic is successful in fixing the issue #570. Also AlvaroVega, has already approved this PR and left the comment LGTM in comment #574 (review)

[Gauravp-NEC]

@fgalan ,Can you merge the code of logic which I have committed in this PR to fix the issue #570? In #577 mapedraza has only added a test that disconnect the DB and check if retrieving data gives error 500 and rest of code logic is copied from my PR. Please let me know your opinion.

[fgalan]

@fgalan ,Can you merge the code of logic which I have committed in this PR to fix the issue #570? In #577 mapedraza has only added a test that disconnect the DB and check if retrieving data gives error 500 and rest of code logic is copied from my PR. Please let me know your opinion.

No, I'm afraid we cannot merge a code which unit tests are nor properly implemented.

Please, have a look to the unit test implemented by @mapedraza in his PR #577. The script of that test looks really nice although it is not currently working (in PR #577 you would see a green mark in GitActions report, but if you run locally the tests in branch task/validate_PR_574, you should see it failing). We are not sure right now if the fail is in the test script or in the STH implementation logic.

Thus, I'd suggest to replace your test in this PR by the test @mapedraza has developed in your PR and continue from that point (i.e. investigating the problem either in the test or in the STH implemented logic).

[Gauravp-NEC]

@fgalan @mapedraza , We have taken two approach to perform UT test case.

1. We have performed three steps as below :
   (i). Connect to database.
   (ii). Close the database.
   (iii). Execute the getCollection(API execution).

Step 1 and 2 are executed successfully but in step 3 , the client.connect() (https://github.com/Gauravp-NEC/fiware-sth-comet/blob/Fix-for-issue-570/lib/database/sthDatabase.js#L320) makes the connection with mongoDB and the positive test result is executed. The expected error 500 scenario is not executed.

2. Secondly, we have performed UT in debug mode, Here also step 1 and 2 are executed successfully and when the debugger reaches to step 3, at that time we have stopped mongoDB maunally and in this scenario client.connect could not make connection with MongoDB and the desired negative test is achieved.

If we can implement mocking in UT test case we can achieve the complete code coverage, but we have observed that mocking is currently not used in STH-Comet. Please let us know your opinion.

[chandradeep11]

@fgalan @mapedraza @AlvaroVega
We have written UT to verify the error scenario of 500 code but this UT is executed successfully when we run the code in debug mode. When debugger reached to function clinet.connect() of sthDatabase.js file then we stopped mongoDB container manually. In this case, expected 500 code returned and UT passed.
But when we run UT without debug mode then 500 code never returned because the calling of client.connect(--) function in else block as mentioned in below code snippet made connection again successfully and returned collection data.

    if (isConnectionAlive()) {
        fetchCollection(databaseName, isAggregated, shouldTruncate, shouldCreate, collectionName, callback);
    } else {
        client.connect(function(err) {
            if(err) {
                return callback(err,null);
            }
        });
    }
}

We request you to provide your input as how can we stop mongoDB database in running UT so that when we push code then CI tool will also execute the UT successfully?
Thanks in advance.

[Gauravp-NEC]

@fgalan @mapedraza @AlvaroVega , We have resolved the UT failure and added new test case to cover error code 500 in commit 921574a.

Now all the test cases have been passed successfully.

[fgalan]

At the current version the PR is almost ready but two things are pending:

1. Not try to reconnect upon connection fail. Already mentioned at Return 500 when DB is not connected (fix for issue 570) #574 (comment) but not fully solved
2. The it('should disconnect from the database', ...) clause is not really a step to test but a preparation statement. It should be done in the "before" clause associated to the containing describe() block.

We are going to merge this PR in an intermediate branch :Fix-for-issue-570-prelanding so @mapedraza could do the final fixes before merging.

Thanks for your contribution!

[fgalan]

Continues in PR #583