Feature/wb50 - Githubissues

vince-a-m commented 1 year ago

Updated DELETE /collection/{id} route to not delete attack objects which are in another collection
Added DELETE /collection/{id}/modified/{modified} to delete a collection by id and version

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 64.15% and project coverage change: +0.25 :tada:

Comparison is base (063c435) 70.14% compared to head (fe73502) 70.40%.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #263 +/- ## =========================================== + Coverage 70.14% 70.40% +0.25% =========================================== Files 118 118 Lines 16044 16085 +41 Branches 2157 2178 +21 =========================================== + Hits 11254 11324 +70 + Misses 4783 4754 -29 Partials 7 7 ``` | [Impacted Files](https://app.codecov.io/gh/center-for-threat-informed-defense/attack-workbench-rest-api/pull/263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=center-for-threat-informed-defense) | Coverage Δ | | |---|---|---| | [app/services/collections-service.js](https://app.codecov.io/gh/center-for-threat-informed-defense/attack-workbench-rest-api/pull/263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=center-for-threat-informed-defense#diff-YXBwL3NlcnZpY2VzL2NvbGxlY3Rpb25zLXNlcnZpY2UuanM=) | `64.15% <62.16%> (+7.90%)` | :arrow_up: | | [app/controllers/collections-controller.js](https://app.codecov.io/gh/center-for-threat-informed-defense/attack-workbench-rest-api/pull/263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=center-for-threat-informed-defense#diff-YXBwL2NvbnRyb2xsZXJzL2NvbGxlY3Rpb25zLWNvbnRyb2xsZXIuanM=) | `67.11% <62.96%> (+6.37%)` | :arrow_up: | | [app/routes/collections-routes.js](https://app.codecov.io/gh/center-for-threat-informed-defense/attack-workbench-rest-api/pull/263?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=center-for-threat-informed-defense#diff-YXBwL3JvdXRlcy9jb2xsZWN0aW9ucy1yb3V0ZXMuanM=) | `100.00% <100.00%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

ElJocko commented 1 year ago

It looks good overall. I have one edge case that needs to be addressed. Plus a couple stylistic things that aren't crucial, but are probably worth doing to keep the code base consistent.

The edge case is when deleting a particular version of a collection with the deleteAllContents=true parameter set. In this situation, the code deletes the objects in the contents of the collection, even if they're still referenced by another version of the same collection. That leaves the remaining version of the collection with dangling references.

Stylistically, there are two things I noticed. The first is the 2 space vs 4 space indentation. I don't have a preference on that, except that I think the code is generally a little easier to read if the indentation is consistent, especially within a file. The second is the way of iterating over a collection. Instead of using for (let i = 0; i < coll.length; i++) the rest of the code uses for (const attackObject of objectList). It's a little more succinct and I think it conveys the intent better.

ElJocko commented 1 year ago

It looks like there's still a problem with deleting the contents when deleting a single version of a collection. I think the issue is that MongoDB uses an implicit AND between query terms. So when modified is set, it's looking for collections that have a different value for stix.id AND a different value for stix.modified. Which will exclude other versions of the same collection.

I pushed a couple of additional test cases that might make it easier to check if things are working the way you want them to.

vince-a-m commented 1 year ago

Thanks for finding that @ElJocko . npm run test ran with no errors