daemon: fix /v2/snaps "Internal Server Error" error when installing unknown snaps

[ZeyadYasser]

Fixes: https://bugs.launchpad.net/snapd/+bug/2024858

Internal Server Error response was returned when only one of many snaps is not found. The problem was passing all snap names (including successful ones) to errToResponse instead of the unknown snap only.

How to reproduce:
In the following example only "spamandeggs" is not found, 404 status code is expected, 500 is returned instead:

curl -sS --unix-socket /run/snapd.socket http://localhost/v2/snaps -X POST \
-d '{"action": "install", "snaps": ["spamandeggs", "hello"]}' -H "Content-Type: application/json" \
| jq

{
  "type": "error",
  "status-code": 500,
  "status": "Internal Server Error",
  "result": {
    "message": "store.SnapNotFound with 2 snaps"
  }
}

Side note:
When multiple snaps are unknown, generic BadRequest 400 is returned (where 404 would be preferable). This is due to the implementation of func (e SnapActionError) SingleOpError() checking that only a single error exists and not single error type exists (e.g. ErrSnapNotFound).
The logic for grouping single type errors already exists here and can be extracted into a reusable function.

curl -sS --unix-socket /run/snapd.socket http://localhost/v2/snaps -X POST \
-d '{"action": "install", "snaps": ["spamandeggs", "spamandeggs2"]}' -H "Content-Type: application/json" \
| jq

{
  "type": "error",
  "status-code": 400,
  "status": "Bad Request",
  "result": {
    "message": "cannot install \"snapandeggs\", \"spamandeggs2\": cannot install: snap not found: \"snapandeggs\", \"spamandeggs2\""
  }
}

[MiguelPires]

The fix looks good, thanks for this. We need a unit test for it as well. Maybe there's even already a test that can be modified/fixed.
Also since this is production code (and not just a change to comments or something like that), the spread tests need to run. I'll close and re-open to trigger them

[ZeyadYasser]

Unit test TestErrToResponse is now updated.
I thought of adding a snaps array instead of the isMany flag for more flexibility in the test, but it seemed very verbose and not necessary. Would love to hear your opinion.

[MiguelPires]

I think it's fine like this. The array would probably remove the need to have the if in the test and support other test cases but for now I think this is good

[codecov-commenter]

Codecov Report

Merging #12945 (15774f3) into master (b0c8a48) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master   #12945      +/-   ##
==========================================
- Coverage   78.61%   78.61%   -0.01%     
==========================================
  Files         995      995              
  Lines      123514   123514              
==========================================
- Hits        97102    97099       -3     
- Misses      20285    20287       +2     
- Partials     6127     6128       +1     

Flag	Coverage Δ
unittests	78.61% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
daemon/errors.go	98.27% <100.00%> (ø)

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[MiguelPires]

LGTM, thanks

[olivercalder]

LGTM, thank you! I was confused by the recursive errToResponse() call (hence the delay in review), but I understand what's going on now. Your changes look great!

[pedronis]

+1