Batch delete

[shroffk]

Addresses issue #213

This PR should be merged after #212

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

Overall Project	13.85% -2.22%	❌
Files changed	36.03%	❌

File	Coverage
SearchParameterMergerUtil.java	100%	🍏
ChannelService.java	44.51% -11.67%	❌
ChannelRepository.java	0.24% -8.01%	❌
ChannelController.java	0% -45.71%	❌
ChannelScrollController.java	0% -76.92%	❌
IChannel.java	0%	❌
IChannelScroll.java	0%	❌

[anderslindho]

This is based on #212 which we still are discussing, so I will ignore those commits.

• The HTTP codes seem wrong: we cannot have 404 here (DELETE /channels exists), and I am hesitant also over 500 on ES failure.
• I am not sure about returning bare (long) count from bulk DELETE, but maybe fine ?
• I don't entirely follow why 2938976 used Refresh.True for deleteAllById.
• Should access on bulk delete be ROLES.CF_CHANNEL? The linked issue says "To address the original safety concerns, access to this endpoint should be restricted to users with a specific role (e.g., CF-ADMIN), ensuring only authorized users can perform bulk deletions."
• e68dbe4 is nice but should maybe have been separate PR; seems to deserve it.

From discussion in #212

However, for the time being I just need these two updates to significantly simplify my workflow. The "new" recceiver was started a few years ago so I thought it would be nice to complete it.

I am still quite hesitant over us including code here in main CF for local development needs. I am fine with it if the extension/code change(s) themselves make sense, but surely there are better ways to improve local dev workflow?

[shroffk]

I can remove 404, maybe a 400 bad request might be more suited for situations where we are trying to delete non-existing channels
I am not sure about the issue with 500?

It is saveall that was using refresh.True.
refresh.True is an expensive operation, in this case I don't think we need immediate visibility of the changes so using refresh.wait_for might be enough. We can change it back to true or make it configurable.

I have updated the Issue, considering this is still a Channel operation I think the current role mapping is correct. It is also consistent with the other batch channel operations.

main CF for local development needs.

These are not changes for local development needs.
Should I bother cleaning up and opening new PR's or is this one a hard no also?

[anderslindho]

I can remove 404, maybe a 400 bad request might be more suited for situations where we are trying to delete non-existing channels

I think 400 also is wrong as it signifies malformed syntax, which this is not. Unfortunately the RFCs don't cover batch, or rather synchronous batch i.e. bulk, but the common three strategies here are:

• idempotency and silent success 20x - doesn't matter if it didn't exist, it now also does not exist; microsoft and others does this
• multi-status 207 (maybe the right fit here if we treat this as non-atomic; we can technically fail for different reasons on different names)
• atomic fail (basically how you treat it now) 422

I am not sure about the issue with 500?

My thought process was that it maybe ought to have been a 502 or 503 instead. I am not sure. I think maybe we really should have both 500 and 503 with Retry-After - the latter for temporary ES failure and fallback 500.

Should I bother cleaning up and opening new PR's or is this one a hard no also?

I am happy with adding the bulk delete feature itself, but I think the point about package location from the linked PR applies also here, and I do not like the inclusion of the additive mix of query params and request body handling. I would personally prefer to see e68dbe4 split out but it's not a blocker.

[anderslindho]

But maybe @jacomago wants to chime in before more work is invested? (He is in training so might be preoccupied.)

[shroffk]

I will split these out into individual PR's