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.

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

add multiple delete of devices #1579

Merged
merged 14 commits into from
Feb 20, 2024
Merged

add multiple delete of devices #1579

merged 14 commits into from
Feb 20, 2024

Conversation

AlvaroVega
Copy link
Member

@AlvaroVega AlvaroVega commented Feb 12, 2024

issue #1578

@fgalan
Copy link
Member

fgalan commented Feb 16, 2024

A delete method with body is not fully restful but is not so ilegal, but we can look for other methods. @fgalan what do you think?

It is not illegal, but not sure if some network middleware (proxies, etc.) could have issues with this. Maybe it's better to avoid this risk using another approach.

Maybe we could use POST /iot/op/delete. This makes explicit that we are not using a REST operation (as we are not acting on a given resource, i.e. a device in IOAT API terms) but a kind of RPC operation (this is similar to the approach used in Orion API with the POST /v2/op operations family).

@AlvaroVega
Copy link
Member Author

A delete method with body is not fully restful but is not so ilegal, but we can look for other methods. @fgalan what do you think?

It is not illegal, but not sure if some network middleware (proxies, etc.) could have issues with this. Maybe it's better to avoid this risk using another approach.

Maybe we could use POST /iot/op/delete. This makes explicit that we are not using a REST operation (as we are not acting on a given resource, i.e. a device in IOAT API terms) but a kind of RPC operation (this is similar to the approach used in Orion API with the POST /v2/op operations family).

Taking a look to resource already used by device API:

function loadContextRoutes(router) {
router.post(
'/iot/devices',
restUtils.checkRequestAttributes('headers', mandatoryHeaders),
restUtils.checkBody(createDeviceTemplate),
handleProvision
);
router.get('/iot/devices', restUtils.checkRequestAttributes('headers', mandatoryHeaders), handleListDevices);
router.get(
'/iot/devices/:deviceId',
restUtils.checkRequestAttributes('headers', mandatoryHeaders),
handleGetDevice
);
router.put(
'/iot/devices/:deviceId',
restUtils.checkRequestAttributes('headers', mandatoryHeaders),
restUtils.checkBody(updateDeviceTemplate),
handleUpdateDevice
);
router.delete('/iot/devices', restUtils.checkRequestAttributes('headers', mandatoryHeaders), handleRemoveDevices);
router.delete(
'/iot/devices/:deviceId',
restUtils.checkRequestAttributes('headers', mandatoryHeaders),
handleRemoveDevice
);
}

I was looking for something like POST /iot/devices/delete

@fgalan
Copy link
Member

fgalan commented Feb 16, 2024

I understand (as seen in commit 9752b31) that at the end we are going to use POST /iot/op/delete.

@AlvaroVega AlvaroVega marked this pull request as ready for review February 16, 2024 14:20
CHANGES_NEXT_RELEASE Outdated Show resolved Hide resolved
doc/api.md Outdated Show resolved Hide resolved
});
});

describe('When a request to remove a provision devices arrives', function () {
Copy link
Member

Choose a reason for hiding this comment

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

Should functional test (i.e. the ones within /test/functional/) to be included?

Copy link
Member Author

@AlvaroVega AlvaroVega Feb 19, 2024

Choose a reason for hiding this comment

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

Not sure if there is support for northbound api in functional tests. @mapedraza is there support for that ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So far, it is not supported northbound only tests (the tests consits of 3 steps, group provision, measure send and CB validation). No so hard to add support adding another "type" of test that check only the northbound using the same test infrastructure. I suggest to merge this like this and create an issue to implement what is describe in this comment.

Copy link
Member

Choose a reason for hiding this comment

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

Issue created: #1581 (please feel free to comment on it if something is missing/wrong)

NTC

Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
doc/api.md Outdated Show resolved Hide resolved
AlvaroVega and others added 2 commits February 19, 2024 16:20
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
Co-authored-by: Fermín Galán Márquez <fgalan@users.noreply.github.com>
doc/api.md Outdated Show resolved Hide resolved
Copy link
Member

@fgalan fgalan left a comment

Choose a reason for hiding this comment

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

LGTM

@fgalan fgalan merged commit bc529ca into master Feb 20, 2024
7 of 8 checks passed
@fgalan fgalan deleted the task/add_multiple_delete_api branch February 20, 2024 08:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants