search

swaggest / rest

Web services with OpenAPI and JSON Schema done quick in Go
https://pkg.go.dev/github.com/swaggest/rest
MIT License
376 stars 17 forks source link

Wrong http status assigned to status.FailedPrecondition #146

Closed pboguslawski closed 1 year ago

pboguslawski commented 1 year ago

Describe the bug status.InvalidArgument and status.FailedPrecondition are different statuses but are mapped to the same http.StatusBadRequest:

https://github.com/swaggest/rest/blob/master/error.go#L130 https://github.com/swaggest/usecase/blob/master/status/status.go#L97

Expected behavior status.FailedPrecondition should be mapped to http.StatusPreconditionFailed.

Additional context Related: #145

vearutop commented 1 year ago

Interesting, original source of mapping is https://github.com/googleapis/googleapis/blob/f36c65081b19e0758ef5696feca27c7dcee5475e/google/rpc/code.proto#L127, I wonder was it a mistake in google repo or deliberate decision.

pboguslawski commented 1 year ago

Seems that FAILED_PRECONDITION description in googleapis is not well mapped to HTTP status code (400 means client side problem) or they have some reason for merging different 4xx errors into one 400.

Consider replacing 400->412 in swaggest for status.FailedPrecondition to allow API clients to distinguish failed preconditions (i.e. client tried to update resource with lost update protection detection and update failed) from bad client requests (i.e. invalid argument that may indicate bug in client).

pboguslawski commented 1 year ago

After this mod, when 304 is added to possible error responses with...

u.SetExpectedErrors(status.Internal, rest.HTTPCodeAsError(http.StatusNotModified))

...openapi.json contains non-empty body for 304 but should not.