Documentation differs from Implementation for /packageManifests/{id} return codes (204 or 404)

[jantari]

Brief description of your issue

Hi,

according to both the 1.1.0 and 1.4.0 REST API schema, the /packageManifests/{PackageIdentifier} route is only supposed to return 200, 404 or another error:

https://github.com/microsoft/winget-cli-restsource/blob/main/documentation/WinGet-1.1.0.yaml#L506-L522
https://github.com/microsoft/winget-cli-restsource/blob/main/documentation/WinGet-1.4.0.yaml#L506-L522

However, the reference implementation also explicitly tests for the undocumented 204 here:

https://github.com/microsoft/winget-cli-restsource/blob/main/src/WinGet.RestSource/Helpers/RestSourceTriggerFunctions.cs#L63

which raises the question - should a REST source return 404 like the schema suggests when a packageIdentifier is not found or should it return 204? I have noticed in testing that winget responds with a red error message when it gets a 404 (which I guess is fine / maybe intended?) but responds with a much more calm "no results found" message when it gets a 204 - so the client seems to handle the undocumented 204 more gracefully, further hinting that that's maybe the intended return code and the schema docs are wrong?

Thanks!

[denelon]

404 is the correct HTTP return code.

We should look to improve the client experience by either parsing the JSON returned by the REST source and returning that to the client, or some other more user-friendly message like "Package not found".

[jantari]

Thanks. Indeed the client-side UX could probably be better then, for reference this is the difference right now:

[jantari]

I'm back because it turns out returning 404 doesn't just result in ugly client-side error messages, it also breaks functionality.

The following has been tested with my own REST source implementation and winget CLI v1.4.3531.

When performing a winget query like:

winget list -s rewinged-local -q bottom

against a REST source, we see the following requests logged by the server:

Starting server on localhost:8443
[GIN-debug] Listening and serving HTTPS on localhost:8443
[GIN] 2023/01/03 - 12:58:14 | 200 |            0s |       127.0.0.1 | GET      "/information"
[GIN] 2023/01/03 - 12:58:14 | 200 |            0s |       127.0.0.1 | GET      "/information"
/packageManifests: Someone tried to GET package ' rewinged.bottom '
with query params: map[]
the package was NOT found!
[GIN] 2023/01/03 - 12:58:15 | 404 |      2.5593ms |       127.0.0.1 | GET      "/packageManifests/rewinged.bottom"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord: MatchType:} Inclusions:[{PackageMatchField:ProductCode RequestMatch:{KeyWord:{af717ccb-7714-4f8d-aece-b03262d97663} MatchType:Exact}} {PackageMatchField:NormalizedPackageNameAndPublisher RequestMatch:{KeyWord:bottom MatchType:Exact}}] Filters:[]}
advanced search with inclusions[] and/or filters[]
Adding to the results map: Clement.bottom version 0.7.0
... with 1 results:
  package Clement.bottom with 1 versions.
&{Data:[{PackageIdentifier:Clement.bottom PackageName:bottom Publisher:Clement Tsang Versions:[{PackageVersion:0.7.0 Channel: PackageFamilyNames:[] ProductCodes:[{DB5C53EB-91B7-4E48-A5BE-DE006CA9C72D}]}]}] RequiredPackageMatchFields:[] UnsupportedPackageMatchFields:[]}
[GIN] 2023/01/03 - 12:58:15 | 200 |      6.3704ms |       127.0.0.1 | POST     "/manifestSearch"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord: MatchType:} Inclusions:[{PackageMatchField:ProductCode RequestMatch:{KeyWord:steam app 969990 MatchType:Exact}} {PackageMatchField:NormalizedPackageNameAndPublisher RequestMatch:{KeyWord:spongebobsquarepantsbattleforbikinibottomrehydrated MatchType:Exact}}] Filters:[]}
advanced search with inclusions[] and/or filters[]
... with 0 results:
[GIN] 2023/01/03 - 12:58:15 | 204 |       1.583ms |       127.0.0.1 | POST     "/manifestSearch"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord:bottom MatchType:Substring} Inclusions:[] Filters:[]}
someone searched the repo for: bottom
... with 1 results:
  package Clement.bottom with 1 versions.
&{Data:[{PackageIdentifier:Clement.bottom PackageName:bottom Publisher:Clement Tsang Versions:[{PackageVersion:0.7.0 Channel: PackageFamilyNames:[] ProductCodes:[{DB5C53EB-91B7-4E48-A5BE-DE006CA9C72D}]}]}] RequiredPackageMatchFields:[] UnsupportedPackageMatchFields:[]}
[GIN] 2023/01/03 - 12:58:15 | 200 |      6.4906ms |       127.0.0.1 | POST     "/manifestSearch"

This results in the following output from winget-CLI:

Failed when searching source; results will not be included: rewinged-local
Name                                                         Id               Version Available
-----------------------------------------------------------------------------------------------
bottom                                                       Clement.bottom   0.6.8   0.7.0
SpongeBob SquarePants: Battle for Bikini Bottom - Rehydrated Steam App 969990 Unknown

Unfortunately, the winget-CLI automatically falls back to the official winget package source to find the 0.7.0 update even though we've explicitly passed the argument to only search our own internal source. This is of course a bug, but not the one I'm getting at here. The issue is clearer when using the PowerShell cmdlets which behave correctly:

> Get-WinGetPackage -Source rewinged-local -Query bottom
Get-WinGetPackage: An error occurred while searching for packages: InternalError

So the PowerShell cmdlet just outright fails and doesn't return any result (at least that part is working).

However, we can see that when winget-CLI does auto-fallback to the official winget source it does find a match and the REST server request logs show that while the initial query from the client to /packageManifests/rewinged.bottom was met with a 404 the client did send another query to /manifestSearch with PackageMatchFields "ProductCode" and "NormalizedPackageNameAndPublisher" that did return the expected package ( Clement.bottom 0.7.0) and a 200 HTTP status code. However, both the winget CLI and the PowerShell cmlets ignore this result and immediately discard the REST source from the results after the initial 404.

If I change the REST server to return 204 instead of 404 when a /packageManifests/:xxx: is not found then we get the correct behavior.

With PowerShell:

> Get-WinGetPackage -Source rewinged-local -Query bottom

Name                             Id                                        Version                                  Available                                Source
----                             --                                        -------                                  ---------                                ------
bottom                           Clement.bottom                            0.6.8                                    0.7.0                                    rewinged-local
SpongeBob SquarePants: Battle f… Steam App 969990                          Unknown

With the old-school CLI:

> winget list -s rewinged-local -q bottom
Name                                                         Id               Version Available
-----------------------------------------------------------------------------------------------
bottom                                                       Clement.bottom   0.6.8   0.7.0
SpongeBob SquarePants: Battle for Bikini Bottom - Rehydrated Steam App 969990 Unknown

Just for completeness, the REST server request logs are now:

Starting server on localhost:8443
[GIN-debug] Listening and serving HTTPS on localhost:8443
[GIN] 2023/01/03 - 13:12:00 | 200 |      4.2049ms |       127.0.0.1 | GET      "/information"
[GIN] 2023/01/03 - 13:12:00 | 200 |            0s |       127.0.0.1 | GET      "/information"
/packageManifests: Someone tried to GET package ' rewinged.bottom '
with query params: map[]
the package was NOT found!
[GIN] 2023/01/03 - 13:12:00 | 204 |      2.2904ms |       127.0.0.1 | GET      "/packageManifests/rewinged.bottom"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord: MatchType:} Inclusions:[{PackageMatchField:ProductCode RequestMatch:{KeyWord:{af717ccb-7714-4f8d-aece-b03262d97663} MatchType:Exact}} {PackageMatchField:NormalizedPackageNameAndPublisher RequestMatch:{KeyWord:bottom MatchType:Exact}}] Filters:[]}
advanced search with inclusions[] and/or filters[]
Adding to the results map: Clement.bottom version 0.7.0
... with 1 results:
  package Clement.bottom with 1 versions.
&{Data:[{PackageIdentifier:Clement.bottom PackageName:bottom Publisher:Clement Tsang Versions:[{PackageVersion:0.7.0 Channel: PackageFamilyNames:[] ProductCodes:[{DB5C53EB-91B7-4E48-A5BE-DE006CA9C72D}]}]}] RequiredPackageMatchFields:[] UnsupportedPackageMatchFields:[]}
[GIN] 2023/01/03 - 13:12:00 | 200 |      5.1233ms |       127.0.0.1 | POST     "/manifestSearch"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord: MatchType:} Inclusions:[{PackageMatchField:ProductCode RequestMatch:{KeyWord:steam app 969990 MatchType:Exact}} {PackageMatchField:NormalizedPackageNameAndPublisher RequestMatch:{KeyWord:spongebobsquarepantsbattleforbikinibottomrehydrated MatchType:Exact}}] Filters:[]}
advanced search with inclusions[] and/or filters[]
... with 0 results:
[GIN] 2023/01/03 - 13:12:01 | 204 |      7.7663ms |       127.0.0.1 | POST     "/manifestSearch"
{MaximumResults:0 FetchAllManifests:false Query:{KeyWord:bottom MatchType:Substring} Inclusions:[] Filters:[]}
someone searched the repo for: bottom
... with 1 results:
  package Clement.bottom with 1 versions.
&{Data:[{PackageIdentifier:Clement.bottom PackageName:bottom Publisher:Clement Tsang Versions:[{PackageVersion:0.7.0 Channel: PackageFamilyNames:[] ProductCodes:[{DB5C53EB-91B7-4E48-A5BE-DE006CA9C72D}]}]}] RequiredPackageMatchFields:[] UnsupportedPackageMatchFields:[]}
[GIN] 2023/01/03 - 13:12:01 | 200 |      7.0523ms |       127.0.0.1 | POST     "/manifestSearch"

So it appears that returning 404 from /packageManifests/:xxx: is in fact incorrect and leads to incorrect winget client behavior.

[jantari]

It looks like @palenshus had already noticed this as well: #107

So basically, the REST implementation in both the winget-client and in the reference-restsource is wrong, it should be a 404, but because the client doesn't handle and doesn't expect 404s correctly and because the reference implementation doesn't use 404s correctly either, one is forced to implement the entirely undocumented 204 response 🤦

[denelon]

Yes, based on https://learn.microsoft.com/azure/architecture/best-practices/api-design#get-methods we should return 404, and the client should handle it correctly. We may need to adjust the reference implementation as well if it's handing out 204.

[denelon]

winget list will display matching results from any configured source. We've got a separate feature on the client to change the behavior of list to only show matching results from a source if one is provided. We may need to treat that as a breaking change though.

Related to:

• Enhancements to list command winget-cli#1155

[zachcarp]

I tend to agree with @denelon. Reading the api-design methods for get reference above:

A successful GET method typically returns HTTP status code 200 (OK). If the resource cannot be found, the method should return 404 (Not Found).

If the request was fulfilled but there is no response body included in the HTTP response, then it should return HTTP status code 204 (No Content); for example, a search operation yielding no matches might be implemented with this behavior.

If we are not finding the resource - 404 is correct (all the references above are non-search based) and we need to correct that in the reference. The one exception would be on manifestSearch - 204 would make sense if we successfully perform a search that returns no results - which appears to be functioning as intended.

The second part of your statement is also true - we need to do some updates on the client to parse/process 404 errors better.