Added feature config groups #1375

KeshavSoni2511 · 2023-05-25T10:18:20Z

Fix for issue #752

KeshavSoni2511 · 2023-05-26T07:48:17Z

Hi @fgalan , the tests are passing on my local setup but here the tests are failing. Kindly look into the issue or anything I can fix. Thanks

mapedraza · 2023-06-06T09:06:14Z

Hi @fgalan , the tests are passing on my local setup but here the tests are failing. Kindly look into the issue or anything I can fix. Thanks

Hi @Keshav-NEC , thank for your willingness to contribute!

The tests only fails on Node V18, which version are you using in your local enviroment?. Would be great if you can review it using the same version (V18). I suggest to use n Node version manager to switch between different node versions

Probably those errors are related with #1333

KeshavSoni2511 · 2023-06-07T08:20:23Z

Hi @mapedraza , I have tested the setup on Node V16. I will test the setup on Node V18 and will share my findings.

KeshavSoni2511 · 2023-06-26T05:47:39Z

Hi @fgalan , @mapedraza I have tested the setup on Node V18 and npm V9.5.1 and test cases are passing and also I have used the link n Node version manager shared by @mapedraza . Please find the logs of test cases of pull request #1375 and also I have attached the screenshot of passing test cases. Thanks.

test/unit/mongodb/mongodb-configGroup-registry-test.js

test/unit/ngsiv2/provisioning/device-provisioning-configGroup-api_test.js

test/unit/ngsiv2/provisioning/singleGroupConfigurationMode-test.js

mapedraza · 2023-06-26T12:06:29Z

As mentioned in my previous comment, you did not addressed the changes made in #1333 in your new tests.

Take as an example the ones I noted above and check the rest of the codebase (I suggested 3, the tests indicates 5, so 2 are remaining)

…t.js Co-authored-by: mapedraza <40356341+mapedraza@users.noreply.github.com>

KeshavSoni2511 · 2023-06-27T05:45:09Z

As mentioned in my previous comment, you did not addressed the changes made in #1333 in your new tests.

Take as an example the ones I noted above and check the rest of the codebase (I suggested 3, the tests indicates 5, so 2 are remaining)

Hi @mapedraza , Thanks for the help and I have done changes as suggested by you and I am sharing the test cases screenshot.

KeshavSoni2511 · 2023-07-20T06:34:15Z

Hi @mapedraza, If you got any chance to review this PR.

mapedraza · 2023-08-11T16:04:14Z

There are still tests failling. You need to solve them before merging this PR

mapedraza

As general comment, all the logic is duplicated for /iot/services and /iot/configGroups. It is quite more easy. The logic should be the same, it is enough routing /iot/configGroups to same methods than /iot/services

mapedraza · 2023-09-14T15:03:54Z

lib/services/northBound/deviceGroupAdministrationServer.js

+function loadConfigContextRoutes(router) {
+    router.post(
+        '/iot/configGroups',
+        restUtils.checkRequestAttributes('headers', mandatoryHeaders),
+        restUtils.checkBody(templateConfigGroup),
+        handleCreateDeviceGroup
+    );
+
+    router.get(
+        '/iot/configGroups',
+        restUtils.checkRequestAttributes('headers', mandatoryHeaders),
+        handleListDeviceGroups
+    );
+
+    router.put(
+        '/iot/configGroups',
+        restUtils.checkRequestAttributes('headers', mandatoryHeaders),
+        restUtils.checkRequestAttributes('query', mandatoryParameters),
+        handleModifyDeviceGroups
+    );
+
+    router.delete(
+        '/iot/configGroups',
+        restUtils.checkRequestAttributes('headers', mandatoryHeaders),
+        restUtils.checkRequestAttributes('query', mandatoryParameters),
+        handleDeleteDeviceGroups
+    );
+}
+


The route is uppercase aware? Did you try requesting to /iot/configgroups?

mapedraza · 2023-09-14T15:05:15Z

doc/api.md

+```json
+{
+    "configGroups": [
+        {
+            "resource": "/deviceTest",
+            "apikey": "801230BJKL23Y9090DSFL123HJK09H324HV8732",
+            "type": "Light",
+            "trust": "8970A9078A803H3BL98PINEQRW8342HBAMS",
+            "cbHost": "http://orion:1026",
+            "commands": [{ "name": "wheel1", "type": "Wheel" }],
+            "attributes": [
+                {
+                    "name": "luminescence",
+                    "type": "Integer",
+                    "metadata": {
+                        "unitCode": { "type": "Text", "value": "CAL" }
+                    }
+                }
+            ],
+            "lazy": [{ "name": "status", "type": "Boolean" }]
+        },
+        {
+            "resource": "/deviceTest2",
+            "apikey": "A21323ASDG12312ASDN21LWQEJPO2J123",
+            "type": "Switch",
+            "trust": "8970A9078A803H3BL98PINEQRW8342HBAMS",
+            "cbHost": "http://orion:1026",
+            "commands": [{ "name": "on", "type": "order" }],
+            "attributes": [
+                {
+                    "name": "swithc_status",
+                    "type": "boolean"
+                }
+            ]
+        }
+    ]
+}
+```
+


This should replace the existing example, not adding a new one.

Fixed in 1178705

mapedraza · 2023-09-14T15:05:47Z

doc/api.md

+
+```json
+{
+    "configGroups": [
+        {
+            "resource": "/deviceTest",
+            "apikey": "801230BJKL23Y9090DSFL123HJK09H324HV8732",
+            "type": "Light",
+            "trust": "8970A9078A803H3BL98PINEQRW8342HBAMS",
+            "cbHost": "http://orion:1026",
+            "commands": [{ "name": "wheel1", "type": "Wheel" }],
+            "attributes": [
+                {
+                    "name": "luminescence",
+                    "type": "Integer",
+                    "metadata": {
+                        "unitCode": { "type": "Text", "value": "CAL" }
+                    }
+                }
+            ],
+            "lazy": [{ "name": "status", "type": "Boolean" }]
+        }
+    ]
+}
+```
+OR
+


This should replace the existing example, not adding a new one

Fixed in 1178705

mapedraza · 2023-09-14T15:07:33Z

lib/services/groups/groupRegistryMemory.js

+/**
+ * List all the configGroups created in the IoT Agent. The result, passed as a parameter in the callback
+ * will be an object with two attributes: the services array with the list of groups; and a count
+ * attribute with the total number of groups in the collection.
+ *
+ * @param {Number} service      Service for wich all the configurations want to be retrieved.
+ * @param {Number} limit        Maximum number of entries to return.
+ * @param {Number} offset       Number of entries to skip for pagination.
+ */
+function listConfigGroups(service, limit, offset, callback) {
+    const result = [];
+    let skipped = 0;
+    const filteredGroups = getConfigurationsByService(service);
+
+    for (const i in filteredGroups) {
+        if (registeredGroups.hasOwnProperty(filteredGroups[i])) {
+            if (offset && skipped < parseInt(offset, 10)) {
+                skipped++;
+            } else {
+                result.push(registeredGroups[filteredGroups[i]]);
+            }
+
+            if (limit && result.length === parseInt(limit, 10)) {
+                break;
+            }
+        }
+    }
+
+    callback(null, {
+        count: filteredGroups.length,
+        configGroups: result
+    });
+}
+


This is duplicated, same as function listGroups(service, limit, offset, callback) . No needed to duplicate code

Fixed in 1178705

mapedraza · 2023-09-14T15:08:06Z

lib/services/groups/groupRegistryMemory.js

+function findConfigGroups(service, subservice, callback) {
+    if (config.getConfig().singleConfigurationMode === true) {
+        return findSingleConfigurationMode(service, subservice, callback);
+    }
+    const result = [];
+
+    for (const i in registeredGroups) {
+        if (
+            registeredGroups.hasOwnProperty(i) &&
+            registeredGroups[i].service === service &&
+            registeredGroups[i].subservice === subservice
+        ) {
+            result.push(registeredGroups[i]);
+        }
+    }
+
+    if (result.length > 0) {
+        return callback(null, {
+            count: result.length,
+            configGroups: result
+        });
+    } else {
+        return callback(new errors.DeviceGroupNotFound(service, subservice));
+    }
+}
+


This is duplicated, same as function find(service, subservice, callback). No needed to duplicate code

Fixed in 1178705

mapedraza · 2023-09-14T15:09:07Z

lib/services/groups/groupRegistryMongoDB.js

+/**
+* List all the configGroups created in the IoT Agent.
+*
+* @param {Number} service      Service for wich all the configurations want to be retrieved.
+* @param {Number} limit        Maximum number of entries to return.
+* @param {Number} offset       Number of entries to skip for pagination.
+*/
+function listConfigGroups(service, limit, offset, callback) {
+    const condition = {};
+
+    function toObjectFn(obj) {
+        return obj.toObject();
+    }
+
+    if (service) {
+        condition.service = service;
+    }
+
+    const query = Group.model.find(condition).sort();
+
+    if (limit) {
+        query.limit(parseInt(limit, 10));
+    }
+
+    if (offset) {
+        query.skip(parseInt(offset, 10));
+    }
+
+    async.series([query.exec.bind(query), Group.model.countDocuments.bind(Group.model, condition)], function (
+        error,
+        results
+    ) {
+        callback(error, {
+            count: results[1],
+            configGroups: results[0].map(toObjectFn)
+        });
+    });
+}
+


Duplicated code, same as function listGroups(service, limit, offset, callback). It is not required.

Fixed in 1178705

mapedraza · 2023-09-14T15:09:47Z

lib/services/groups/groupRegistryMongoDB.js

+/**
+* List all the configGroups created in the IoT Agent for a service and a subservice.
+*
+* @param  {String} service        Service used to filter the groups.
+* @param  {String} subservice     Subservice used to filter the groups.
+* @param  {Function} callback     The callback function.
+*/
+function findConfigGroups(service, subservice, callback) {
+   const condition = {};
+
+    function toObjectFn(obj) {
+        return obj.toObject();
+    }
+
+    condition.service = service;
+    condition.subservice = subservice;
+
+    const query = Group.model.find(condition).sort();
+
+    async.series([query.exec.bind(query), Group.model.countDocuments.bind(Group.model, condition)], function (
+        error,
+        results
+    ) {
+        callback(error, {
+            count: results[1],
+            configGroups: results[0].map(toObjectFn)
+        });
+    });
+}
+


Duplicated code, same as function find(service, subservice, callback) . No required

Fixed in 1178705

mapedraza · 2023-09-14T15:10:24Z

lib/services/groups/groupRegistryMongoDB.js

 exports.list = alarmsInt(constants.MONGO_ALARM + '_02', intoTrans(context, listGroups));
+exports.listConfigGroups = alarmsInt(constants.MONGO_ALARM + '_12', intoTrans(context, listConfigGroups));
 exports.init = alarmsInt(constants.MONGO_ALARM + '_03', intoTrans(context, init));
 exports.find = alarmsInt(constants.MONGO_ALARM + '_04', intoTrans(context, find));
+exports.findConfigGroups = alarmsInt(constants.MONGO_ALARM + '_13', intoTrans(context, findConfigGroups));
 exports.findType = alarmsInt(


Exporting duplicated functions, not required

Fixed in 1178705

mapedraza · 2023-09-14T15:12:26Z

lib/services/northBound/deviceGroupAdministrationServer.js

+    if (req.body.configGroups) {
+        for (let i = 0; i < req.body.configGroups.length; i++) {
+            req.body.configGroups[i] = applyMap(apiToInternal, req.body.configGroups[i]);
+            req.body.configGroups[i].service = req.headers['fiware-service'];
+            req.body.configGroups[i].subservice = req.headers['fiware-servicepath'];
+        }
+    } else {    
+        for (let i = 0; i < req.body.services.length; i++) {
+            req.body.services[i] = applyMap(apiToInternal, req.body.services[i]);
+            req.body.services[i].service = req.headers['fiware-service'];
+            req.body.services[i].subservice = req.headers['fiware-servicepath'];
+        }
+   }


The logic of different routes ( /iot/services and /iot/configGroups should be the same)

Fixed in 1178705

mapedraza · 2023-09-14T15:16:02Z

@Keshav-NEC thank you so mucho for your contribution. I think it is quite easy to change the PR according to my feedback. Please, address all the comments and the general idea before reviewing again

KeshavSoni2511 · 2023-09-22T03:59:26Z

Hi @mapedraza, As in this comment, #752 (comment) it is suggested to keep the backward compatibility intact.
I have created the separate functions for /iot/services and /iot/configGroups to return the result in services:[] and configGroups:[] objects respectively.
listGroups() will output the result into services: [] object here but listConfigGroups() will output the result into configGroups:[] object here.

As per @mapedraza, I need to delete the listConfigGroups() and other functions which are similar to previous one. And handle both APIs /iot/services and /iot/configGroups in the same function by using if/else or some other logic. Could you please confirm my understanding?

mapedraza · 2023-09-22T14:29:58Z

@Keshav-NEC as you stated, the idea is to not break previous methods and paths. My comment comes because of the duplicated logic you added:

function listGroups(service, limit, offset, callback) {
    const result = [];
    let skipped = 0;
    const filteredGroups = getConfigurationsByService(service);
    for (const i in filteredGroups) {
        if (registeredGroups.hasOwnProperty(filteredGroups[i])) {
            if (offset && skipped < parseInt(offset, 10)) {
                skipped++;
            } else {
                result.push(registeredGroups[filteredGroups[i]]);
            }
            if (limit && result.length === parseInt(limit, 10)) {
                break;
            }
        }
    }
    callback(null, {
        count: filteredGroups.length,
        services: result
    });
}

function listConfigGroups(service, limit, offset, callback) {
    const result = [];
    let skipped = 0;
    const filteredGroups = getConfigurationsByService(service);

    for (const i in filteredGroups) {
        if (registeredGroups.hasOwnProperty(filteredGroups[i])) {
            if (offset && skipped < parseInt(offset, 10)) {
                skipped++;
            } else {
                result.push(registeredGroups[filteredGroups[i]]);
            }

            if (limit && result.length === parseInt(limit, 10)) {
                break;
            }
        }
    }

    callback(null, {
        count: filteredGroups.length,
        configGroups: result
    });
}

Both codeblocks mentioned above differs just in one line, the name of the variable passed to the callback. Probably there are better ways to implement it without duplicating code, that supposes worst maintainability (duplicating logics and paths). There are different alternatives to achieve this, like controlling it at higher level, or a wrapping the function. Take the one you feel confident with it, but try not to duplicate the code.

AlvaroVega · 2023-09-27T08:04:12Z

What is the difference between this PR and these others: #1198 and #954.
I guess two of there PRs should be closed.

mapedraza · 2023-09-27T14:00:38Z

What is the difference between this PR and these others: #1198 and #954. I guess two of there PRs should be closed.

Older PRs closed

Merge remote-tracking branch 'upstream/master' into Added_Feature_ConfigGroups

KeshavSoni2511 · 2023-10-17T10:51:01Z

Hi @mapedraza , I have updated the PR as per the review comments received. Please review, Thanks.

Line 62 removed and rebased

KeshavSoni2511 · 2024-02-27T19:14:19Z

Hi @mapedraza, If you got any chance to review this PR.

KeshavSoni2511 · 2024-04-25T09:53:39Z

Hi @fgalan, if you got any chance to review this PR

mapedraza

Hi @KeshavSoni2511, than you so much for your amazing job. Now it is more aligned. Just some minor improvements points:

1): I can't see tests implementing cross functionality. I mean, creating groups using services and then retrieving it through configGroup endpoint. It should be aded in both ways, I mean, creating as services and configGroup and retrieving them with the opposite.
2) In order to configure the keyword used by the new endpoint, it would be great if instead of having a magic word, it is moved to the constans file (lib/constants.js), replacing each time in the code appears 'configgroups'. It should be configured for the API route and for the term used in the json response.

mapedraza · 2024-08-06T13:36:47Z

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it

Thanks in advance!

Hi @KeshavSoni2511, than you so much for your amazing job. Now it is more aligned. Just some minor improvements points:

1): I can't see tests implementing cross functionality. I mean, creating groups using services and then retrieving it through configGroup endpoint. It should be aded in both ways, I mean, creating as services and configGroup and retrieving them with the opposite.

2) In order to configure the keyword used by the new endpoint, it would be great if instead of having a magic word, it is moved to the constans file (lib/constants.js), replacing each time in the code appears 'configgroups'. It should be configured for the API route and for the term used in the json response.

KeshavSoni2511 · 2024-08-07T04:36:15Z

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it

Thanks in advance!

Hi @mapedraza, Hope you are doing well. FYI, due to sudden layoff, I am not working with NEC anymore. I think anyone else from NEC can do requested changes.

…Groups

mapedraza · 2024-08-07T08:03:40Z

Hi @KeshavSoni2511, could you address what I noted in my previous comment?. It would be great if you complete everything so we can merge it
Thanks in advance!

Hi @mapedraza, Hope you are doing well. FYI, due to sudden layoff, I am not working with NEC anymore. I think anyone else from NEC can do requested changes.

Really sad to hear that. I will merge your PR into a prelanding branch, so the work you accomplished is not going to be be thrown.

Thank for your help

root and others added 7 commits May 22, 2023 09:51

Fix_for_nodelib_issue_752

4aa64c5

Fix for issue 752

1016def

Document improvements for issue 752

ab1d728

Syncing with master

0b0973d

Rebase

1ea983f

Update deviceConfigGroup.json

5c686c0

Update deviceService-test.js

5899a2b

mapedraza reviewed Jun 26, 2023

View reviewed changes

test/unit/mongodb/mongodb-configGroup-registry-test.js Show resolved Hide resolved

test/unit/ngsiv2/provisioning/device-provisioning-configGroup-api_test.js Show resolved Hide resolved

test/unit/ngsiv2/provisioning/singleGroupConfigurationMode-test.js Outdated Show resolved Hide resolved

KeshavSoni2511 and others added 2 commits June 27, 2023 10:15

Update test/unit/ngsiv2/provisioning/singleGroupConfigurationMode-tes…

a4db57a

…t.js Co-authored-by: mapedraza <40356341+mapedraza@users.noreply.github.com>

Latest Changes

4fad7ea

Latest Changes

a07215c

KeshavSoni2511 added 3 commits August 14, 2023 04:59

Rebased

207cbb5

ChangeAccToMaster

5174f01

Rebased

92340d4

mapedraza requested changes Sep 14, 2023

View reviewed changes

KeshavSoni2511 added 2 commits September 20, 2023 23:32

Rebased and latest changes done

2abd610

Rebased

06bcc74

mapedraza mentioned this pull request Sep 27, 2023

Added Feature/config groups #1198

Closed

mapedraza mentioned this pull request Sep 27, 2023

Added Feature/config groups #954

Closed

KeshavSoni2511 added 2 commits October 17, 2023 16:04

Remove_Code_Duplicacy

7483358

q

c0100be

Merge remote-tracking branch 'upstream/master' into Added_Feature_ConfigGroups

KeshavSoni2511 and others added 3 commits October 17, 2023 17:45

FixLintError

8f91d13

Update groupService.js

1178705

Line 62 removed and rebased

Rebased_with_latest_changes

7320d9a

KeshavSoni2511 requested a review from mapedraza January 30, 2024 12:47

Rebased and resolved conflicts

407c156

mapedraza requested changes Apr 25, 2024

View reviewed changes

mapedraza changed the base branch from master to prelanding/new-config-groups August 7, 2024 07:55

Merge branch 'prelanding/new-config-groups' into Added_Feature_Config…

bc1a1d4

…Groups

mapedraza merged commit e53d004 into telefonicaid:prelanding/new-config-groups Aug 7, 2024

mapedraza mentioned this pull request Aug 7, 2024

[Do not merge] New config groups API endpoint #1637

Closed

1 task

Added feature config groups #1375

Added feature config groups #1375

Conversation

KeshavSoni2511 commented May 25, 2023

KeshavSoni2511 commented May 26, 2023 • edited Loading

mapedraza commented Jun 6, 2023 • edited Loading

KeshavSoni2511 commented Jun 7, 2023

KeshavSoni2511 commented Jun 26, 2023

mapedraza commented Jun 26, 2023

KeshavSoni2511 commented Jun 27, 2023 • edited Loading

KeshavSoni2511 commented Jul 20, 2023

mapedraza commented Aug 11, 2023

mapedraza left a 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

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

mapedraza commented Sep 14, 2023

KeshavSoni2511 commented Sep 22, 2023

mapedraza commented Sep 22, 2023 • edited Loading

AlvaroVega commented Sep 27, 2023

mapedraza commented Sep 27, 2023

KeshavSoni2511 commented Oct 17, 2023

KeshavSoni2511 commented Feb 27, 2024

KeshavSoni2511 commented Apr 25, 2024

mapedraza left a comment

Choose a reason for hiding this comment

mapedraza commented Aug 6, 2024 • edited Loading

KeshavSoni2511 commented Aug 7, 2024

mapedraza commented Aug 7, 2024

KeshavSoni2511 commented May 26, 2023 •

edited

Loading

mapedraza commented Jun 6, 2023 •

edited

Loading

KeshavSoni2511 commented Jun 27, 2023 •

edited

Loading

mapedraza commented Sep 22, 2023 •

edited

Loading

mapedraza commented Aug 6, 2024 •

edited

Loading