3 ms·
In this particular example, it’s actually a whole lot worse than that, by spec at least. Excerpt from their OpenAPI schema <https://github.com/nginx/unit/blob/7
by chrismorgan 3y ago
In this particular example, it’s actually a whole lot worse than that, by spec at least. Excerpt from their OpenAPI schema <https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ecb403f96981bb4ee/docs/unit-openapi.yaml https://github.com/nginx/unit/blob/7dd5ad93a4c147b086a8d82ec...>:
jsonSuccessMessage:
type: object
description: "JSON message on success."
additionalProperties:
type: string
jsonErrorMessage:
type: object
description: "JSON message on error."
additionalProperties:
type: string
Yes, this is as bad as it looks: “success” isn’t even part of the schema. It’s in the examples, but not the actual schema definition. The way success or failure is actually indicated is by HTTP status codes. 200 is success, 400/404/500 is error. So, they seem to set very bad precedent in at least one of their response and their schema.
But I was expecting it to be something like {"success": string} | {"error": string}, which is frankly a perfectly reasonable way of doing things: an untagged, but still unambiguous, union. It can make for quite pleasant code, too: `if response.success` and such.
You’re looking for a tagged union, something like {"result": "success" | "error", "message": string} (or alternatively like {"result": "success", "message": string} | {"result": "error", "code": string, …}), which is also a perfectly reasonable way of doing things. In some ways it can be viewed as more principled, as it more formally allows you to check the tag.
Really, the two approaches are much of a muchness. They each have their strengths and their weaknesses. But if general status is being done at the HTTP layer, I’d prefer {"message": "Reconfiguration done."}, or… well, actually, just a 204 No Content response, and no JSON or body at all.
(On the terms untagged and tagged unions as I’m using them: they’re necessarily a bit different from the concepts as exposed in languages like C, since you’re dealing with a model built on objects rather than bytes, but they’re still reasonable descriptions of them. In Serde’s classifications, they’d be the untagged and internally tagged enum representations <https://serde.rs/enum-representations.html https://serde.rs/enum-representations.html>.)
- Nullabillity 3y agoBy Serde's classification, `success: {}` is a textbook externally tagged representation.
- mindwok 3y agoI wasn't familiar with Serde's description of this so that was a good read, thanks. Although I still think this is a bad example of an externally tagged representation. In the Serde example they have the key as "Request" then what follows is the request object. In this example, the "success" key is followed by an arbitrary string message, which isn't obvious at all.
- Nullabillity 3y agoThe contents of each variant don't really matter here. The tag represents the variant, not the type contained inside of it. The Serde equivalent here would be: #[derive(Serialize, Deserialize)] #[serde(rename_all = "camelCase")] pub enum Response { Success(serde_json::Value), Error(serde_json::Value), }
- chrismorgan 3y agoIf there was always only one top-level field and everything else was inside that, it could be externally tagged, but it’s definitely just untagged here, because #/components/examples/errorInvalidJson/value (line 4397) shows an error body with multiple top-level fields: {"error":"Invalid JSON.","detail":"…","location":{"offset":0,"line":1,"column":0}}. Externally tagged would be {"error":{"message":"Invalid JSON.","detail":"…","location":{…}}}.
- thibaut_barrere 3y ago> Yes, this is as bad as it looks: “success” isn’t even part of the schema. It’s in the examples, but not the actual schema definition. Having gone through a pretty heavy overhaul of an "OpenAPI" (full code in Elixir at https://github.com/etalab/transport-site/pull/3351 https://github.com/etalab/transport-site/pull/3351), I stumbled on that exact type of problem! At the scale of nginx, having automatic verification that the examples (and the output of the API in general) match the specification would be great. At our scale, here is what really helped me go through the rework (and ensure we do not regress too easily): - setting additionalProperties to "false" to detect key field "rot" - using "required: [x,y,z]" on everything, and by default specify "all the property keys", with an opt-out (so that each time a developer adds a field later, it is considered mandatory, unless otherwise specified) - use tooling during the tests: "assert_schema" (with OpenAPISpex) to ensure our API endpoints responses pass the spec (additionalProperties: false helps ensure we get an exception in case of key field rot, again!) - even more useful: crawl our production most important endpoints and tweak the spec until everything is green (an example of useful use of Task.async_stream in Elixir, by the way) https://github.com/etalab/transport-site/pull/3351/files#diff-e73c888e72cf3a5df29a7b628cb5ad208324e989ad8cc2cc5f7d12dce89a9707 https://github.com/etalab/transport-site/pull/3351/files#dif... It can be super frustrating for users to live with the uncertainty of the response of an API for sure, and I was happy to discover the Elixir tooling (OpenAPISpex in particular) worked so nicely once I understood what I had to do.
- tough 3y agoI recently found this open source postman typed alternative, hadn't had time to really try it yet but the types stuff should help here no? https://recipeui.com/ https://recipeui.com/
- thibaut_barrere 3y agoThe type support in OpenAPI is fairly good (enough for us), and using a tool directly built on our stack allows nice things. Thanks for the link though!
- 3y ago
- usrusr 3y agoLooks like the openAPI is a little rough around the redundant status fillers? Wouldn't be the first one I guess. Putting the redundancy with 200 or not 200 aside, I sense a certain aesthetic quality in the {"success":string}|{"error":string} approach. Namely in how it adheres to keeping the schematic stuff on left side of the colon.