Fiqare secmotic rules #411

fiqare-secmotic · 2019-12-16T10:42:07Z

This pull request contains improvements made by the Secmotic team for iotagent-ul. These improvements are part of the fiQare project, which is based on ISO 25010 to improve software quality. More info: https://fiqare.eu/

There are two rules:

Rule 1: Unused function parameters should be removed, based on:

MISRA C++:2008, 0-1-11 - There shall be no unused parameters (named or unnamed) in nonvirtual functions.
MISRA C:2012, 2.7 - There should be no unused parameters in functions
CERT, MSC12-C. - Detect and remove code that has no effect or is never executed

Rule 2: if.. else if constructs should end with else clauses, based on:

MISRA C:2004, 14.10 - All if...else if constructs shall be terminated with an else clause.
MISRA C++:2008, 6-4-2 - All if...else if constructs shall be terminated with an else clause.
MISRA C:2012, 15.7 - All if...else if constructs shall be terminated with an else statement
CERT, MSC01-C. - Strive for logical completeness
CERT, MSC57-J. - Strive for logical completeness

All test in Travis has been passed successfully and the coverage in Coveralls remains the same.

fgalan · 2019-12-16T19:02:19Z

lib/bindings/HTTPBindings.js

@@ -44,7 +44,7 @@ var http = require('http'),
    },
    transport = 'HTTP';

-function handleError(error, req, res, next) {
+function handleError(error, req, res) {


Not sure if the fix for this is removing next from the signature of the function of this change is actually raising another kind of bug in the function: it should call next() upon completion but it is not doing it.

A similar case would be the one with returnCommands() modification, some lines below.

@AlvaroVega what do you think?

I guess express is expecting always a function with req, res, next) args

I have been discussing this case with @AlvaroVega and the solution here shouldn't be to remove "next" for the parametrs, but adding the next(); call to the end of the function, i.e. just after L59.

fixed in a2e11f6

lib/iotaUtils.js

Co-Authored-By: Fermín Galán Márquez <[email protected]>

fgalan · 2020-01-09T12:35:40Z

lib/iotaUtils.js

@@ -155,7 +155,7 @@ function mergeDeviceWithConfiguration(deviceData, configuration, callback) {
        } else if (!deviceData[fields[i]] && (!configuration || !configuration[confField])) {
            deviceData[fields[i]] = defaults[i];
        } else {
-            config.getLogger().error(context, 'There is no possible merge');
+            config.getLogger().debug(context, 'at field %d configuration merging logic did not merge anything', i);


Is "%d" the right formatter for integer in JavaScript?

Changing log from error to debug level, sure?

Probably you are seeing a partical commit... if you clear filters in diff tab the actual change shown is the addition of the a debug line. I mean, there wasn't any error() logging here before.

you're right

Confirmed "%d" is correct. Thus NTC (I guess)

fixed in a2e11f6

AlvaroVega · 2020-01-13T17:01:23Z

lib/bindings/HTTPBindings.js

@@ -159,7 +159,7 @@ function returnCommands(req, res, next) {
        updates = commandList.map(createCommandUpdate);
        cleanCommands = commandList.map(cleanCommand);

-        async.parallel(updates.concat(cleanCommands), function(error, results) {


results seems a reminder of a log that was necessary.

In that case the fix should be to use results to provide a proper log messages based on it.

In this case we continue to rely on ISO 25010 to improve software quality. Mainly:

Rule 1: Unused function parameters should be removed, based on:

MISRA C++:2008, 0-1-11 - There shall be no unused parameters (named or unnamed) in nonvirtual functions. MISRA C:2012, 2.7 - There should be no unused parameters in functions CERT, MSC12-C. - Detect and remove code that has no effect or is never executed

So, this is fixed in a2e11f6

We agree that unused parameters should be avoided. However, we are proposing a different solution for this case: instead of removing the unused results parameter, let's add a log trace that uses results to print some useful information in the logs for the user.

What do you think?

Based on the maintainability axis of ISO 25010, I think it is counterproductive to add code to the function to use a variable that is currently deprecated, which makes it less readable. Therefore, before adding code that will not be functional, I prefer to leave the function as it was, with the variable "results".

fixed in 9dfbdcb

fgalan · 2020-01-13T17:13:47Z

lib/bindings/HTTPBindings.js

@@ -135,7 +135,7 @@ function checkMandatoryParams(queryPayload) {
 * This middleware checks whether there is any polling command pending to be sent to the device. If there is some,
 * add the command information to the return payload. Otherwise it returns an empty payload.
 */
-function returnCommands(req, res, next) {
+function returnCommands(req, res) {


Similarly to handleError() the solution should be to call next(); in the proper place (I guess that just after L205, but not fully sure).

fixed in a2e11f6

fgalan · 2020-01-13T17:15:08Z

lib/commonBindings.js

@@ -115,7 +115,7 @@ function manageConfigurationRequest(apiKey, deviceId, device, objMessage) {
 * @param {Number} index        Index of the group in the array.
 * @return {Array}             Updated array of functions.
 */
-function processMeasureGroup(device, apikey, previous, current, index) {
+function processMeasureGroup(device, apikey, previous, current) {


JavaDoc entry for index (L115) should be removed also.

fixed in 662a458

fgalan · 2020-01-20T16:33:15Z

lib/iotaUtils.js

+        } else {
+            config
+                .getLogger()
+                .debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything', i);


Suggested change

.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything', i);

.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything');

not fully sure but I'd say that if you don't use formating (such as "%d") then the i variable should not be included at the end of the statement.

fixed in 9dfbdcb

fgalan · 2020-01-21T11:10:14Z

lib/bindings/HTTPBindings.js

@@ -57,6 +57,7 @@ function handleError(error, req, res, next) {
        name: error.name,


I think the PR is almost ready (good work! :). However, you should include an entry in the CHANGES_NEXT_RELEASE file in describing the changes. Maybe something like the following:

- Hardening: software quality improvement based on ISO25010 recommendations

fixed in 484e833

AlvaroVega

LGTM

fgalan

LGTM (at commit 484e833). Thanks for your contribution!

In order to keep the homogeneity along with all the IOTA suite, we will wait to merge this PR until a similar PR with software improvements based on ISO25010 recommendations could be done in the following repositories:

fgalan · 2020-07-03T12:00:55Z

After merging PR #415 I'm afraid some merging conflict have arisen in this PR. Fortunatelly, the solution seems easy, detailed by @jason-fox at telefonicaid/iotagent-manager#171 (comment)

all you need to do is merge and accept yours

Thereafter
npm i
npm run lint
And fix any es6 errors raised. (Alternatively you could disable the failed rule for your files if necessary)

Sorry for the incoveniences

fiqare-secmotic added 2 commits December 16, 2019 11:28

Rule 2: Unused function parameters should be removed

3e79647

Rule 4: if.. else if constructs should end with else clauses

85f7388

fgalan reviewed Dec 16, 2019

View reviewed changes

lib/iotaUtils.js Outdated Show resolved Hide resolved

fgalan mentioned this pull request Dec 17, 2019

Secmotic fiQare PR #407

Closed

fgalan mentioned this pull request Jan 8, 2020

Feature/eslint #408

Closed

Update lib/iotaUtils.js

da9359c

Co-Authored-By: Fermín Galán Márquez <[email protected]>

fgalan reviewed Jan 9, 2020

View reviewed changes

AlvaroVega reviewed Jan 13, 2020

View reviewed changes

fgalan reviewed Jan 13, 2020

View reviewed changes

fiqare-secmotic added 4 commits January 20, 2020 14:22

fiqare secmotic rules imporved

a2e11f6

change branch master to fiqare-secmotic-rules for travis

46f6126

updated to previous version

2e0a51d

JavaDoc entry removed

662a458

fgalan reviewed Jan 20, 2020

View reviewed changes

Variable reset "results" and function "debug()" improved

9dfbdcb

fgalan reviewed Jan 21, 2020

View reviewed changes

AlvaroVega approved these changes Jan 21, 2020

View reviewed changes

new entry for CHANGES_NEXT_RELEASE

484e833

fgalan approved these changes Jan 23, 2020

View reviewed changes

jason-fox mentioned this pull request Jun 22, 2020

Should every IoT Agent align to use eslint telefonicaid/iotagent-node-lib#830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fiqare secmotic rules #411

Fiqare secmotic rules #411

fiqare-secmotic commented Dec 16, 2019

fgalan Dec 16, 2019

AlvaroVega Jan 13, 2020

fgalan Jan 13, 2020

fiqare-secmotic Jan 20, 2020

fgalan Jan 9, 2020

AlvaroVega Jan 13, 2020

fgalan Jan 13, 2020

AlvaroVega Jan 13, 2020 •

edited

Loading

fgalan Jan 13, 2020

fiqare-secmotic Jan 20, 2020

AlvaroVega Jan 13, 2020

fgalan Jan 13, 2020

fiqare-secmotic Jan 20, 2020

fgalan Jan 20, 2020

fiqare-secmotic Jan 21, 2020

fiqare-secmotic Jan 21, 2020

fgalan Jan 13, 2020

fiqare-secmotic Jan 20, 2020

fgalan Jan 13, 2020

fiqare-secmotic Jan 20, 2020

fgalan Jan 20, 2020

fiqare-secmotic Jan 21, 2020

fgalan Jan 21, 2020 •

edited

Loading

fiqare-secmotic Jan 21, 2020

AlvaroVega left a comment

fgalan left a comment •

edited

Loading

fgalan commented Jul 3, 2020

	.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything', i);
	.debug(context, 'at field "' + fields[i] + '" configuration merging logic did not merge anything');

		@@ -57,6 +57,7 @@ function handleError(error, req, res, next) {
		name: error.name,

Fiqare secmotic rules #411

Are you sure you want to change the base?

Fiqare secmotic rules #411

Conversation

fiqare-secmotic commented Dec 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvaroVega Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Jan 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlvaroVega left a comment

Choose a reason for hiding this comment

fgalan left a comment • edited Loading

Choose a reason for hiding this comment

fgalan commented Jul 3, 2020

AlvaroVega Jan 13, 2020 •

edited

Loading

fgalan Jan 21, 2020 •

edited

Loading

fgalan left a comment •

edited

Loading