Hacker News Reader

Comment by mindwok | Hacker News Reader

Well I commend the Nginx team for trying something new and launching this, even though I'm not sure what I would use it for personally.

Slightly tangental, but it always irks me when I see these kinds of responses in JSON:

  {
      "success": "Reconfiguration done."
  }

Really this should be something like "result": "success". Using "success" as a key name tells me nothing about the data it's representing.

View on HN

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/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>.)

By Serde's classification, `success: {}` is a textbook externally tagged representation.

I 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.

The 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),
    }

If 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":{…}}}.

> 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), 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#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.

I 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/

The 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!

AFAIK this is just a frontend Postman replacement which also supports OpenAPI, no idea how much it differs from showing the underlying type support from OpenAPI specs tho, seems to use collections so probably accepts postman ones to import. dunno just saw the other day on show hn

Looks 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.

I think this is trying to be a discriminated union type[1] in json. One way to do it is to have a ‘tag’ field that says “ok” or “err” or whatever and then other fields for the rest of the data (or an array with the tag in the first slot). Another is to have one field whose name is the tag and whose value is everything else (which is what happened here).

[1] eg one where the possible values are either Err(<some error>) or Ok(<some result>), and the data inside could be more complex types instead of just strings

Heh. My pet peeve are useless success fields. A 200 response tells you it was successful already. We can talk about a „message“ field or something if you intend to display it to a user, but even that should be implicit by being a response to a specific request. If I send a POST /configuration, is there really something new to be gained from that message that the client cannot figure out on its own? Wouldn’t, perhaps, a 201 No Content suffice?

Eric S. Raymond, in his guide "The Art of Unix Programming", mentions this principle as Rule of Silence: "When a program has nothing surprising to say, it should say nothing."

His statement probably related to command line applications, but it makes sense for a lot of cases.

It sounds smart but what counts as surprising is entirely context dependent and most programs won't be aware of your context.

E.g. a command line app where you put a subtly wrong switch in that does exactly what it thought you wanted and prints nothing while outputting a 0 exit code is dangerous.

What, like rm -rf ?

It's about context, of course, but it's not really the program's responsibility to know the user's context. It's the user's responsibility to understand their own context.

A tool should have sufficient interlocks to ensure safety when not engaged, but no more.

I follow your point, however it actually supports mine: An HTTP response isn’t intended to be the output of a command line app. A command line app should take the response from the lower-level HTTP communication and articulate whatever appropriate message to the user, depending on the verbosity level for example. Talking in OSI terms - this is a presentation layer problem, not a link layer one.

"When a program has nothing surprising to say, it should say nothing."

Which sounds good in theory, until you run into issues like this: https://www.youtube.com/watch?v=tLdRBsuvVKc

TLDW: Most of the Gitlabs outage where they deleted the primary prod and backup prod database could have been solved if they just waited for the command line op to complete - but because of a lack of feedback they thought it had frozen, leading to several other plans which made the situation worse.

That's like when I see APIs that give you responses with a 200 code, but then in the message there's a backend error.

Looks pointedly at graphQL projects.

Some people treat http as a pure transport/network layer and the actual body as the app-level layer.

In such an approach, any http-level error would be a network, server maintenance or other unknown/fatal error type of things.

App errors are encoded in the response body in a app-defined way.

It's not a completely bogus way of handling things, as long as it is perfectly consistent throughout the project and properly documented, which is rarely the case.

The opposite of that is people who try to loosely project the variety of error modes their app has to http status codes. You then print out a table of http code <-> actual meaning and try to outline ranges and messages you want to handle differently. On top of that, their backend may be down and you have to deal with bare reverse proxy statuses as well, which adds another dimension to that mess.

Unit has been around for half a decade. It feels like an evolution to something like Phusion Passenger, but it's not quite cloud-native. A lot of the documentation is tailored to installing directly on a server and some essentials (i.e. prometheus metrics) are missing.

I briefly evaluated it for bringing a PHP team into our Kubernetes cluster, but then ended up writing a bit of Go code to proxy into a real nginx+fcgi while adding a syslog sink (so PHP could log to stdout/stderr) and our standard prometheus metrics on the Go http server.

wait, did that continue to use php-fpm? if not I want details plz! :D (especially around the logging sync, unless it’s just php-fpm configured to collect child process output)

Yes, it did, not too much magic going on, the Go proxy also was a replacement for supervisord so it started nginx and fpm, reaped zombies and pulled all the important logs into stdout/stderr. The most remarkable thing was how this revitalized a team that wrote boring PHP software limited to deploying via FTP on PHP 5.

They ended up really getting into stripping out every piece of PHP they didn't use because the image built PHP from scratch, eventually took over maintenance of the go piece and ported a bunch of their apps to Symphony. I was in a platform engineering team and they were one of the few teams to really torture test every feature we ever shipped to the point they'd report edge cases or a bug to us every other week or so.

As for the logging question, we configured Symphony to log to syslog, which was provided by the Go daemon via unix socket.

awesome, thank you for the details!

depending on the desired metrics, the Unit /status endpoint may provide enough info, and transformation of json to prometheus metrics is somewhat easy with existing tooling

Regarding use case. We have a bunch of frontend apps that are integrations to one of our external platforms - they get hosted in an iframe on there.

To simplify the builds we have a expressjs app that is a simple static server and proxy for API requests.

I can see something like this easily replace that - all we need is static files and a /API* endpoint to proxy - avoiding cors issues.

Should it be

{ status: 200, body: { "message": "Reconfiguration done." } }

I rather have:

{ "responseMessage": "Reconfiguration done.", "responseCode": 133, "isSuccessful": true }

This way you can map to a custom message on your front-end based on the responseCode whether its successful or not, you can fallback to the response message if no mapping is found, and you can easily check if the transaction was successful or not.

If I were designing it I’d probably have a result field too, or even a success boolean. But I don’t hate what they’re doing here: the presence of the key is the Boolean value, the value is the description. A two for one.

An economy not worth investing in in my book. It’s always more clear to separate success and result. Sometimes, you’ll have no result, so there will be some parasitic (or truthy) value like null or true. And when you check success-named result deeper/further in code, it looks like you’re indexing into a boolean. Trying to make it clear on-site creates miniprotocols not worth remembering. Worst case scenario is {success:x, error:y}, x and y being 4 combinations of null and non-null, where you aren’t even sure what happened and may swallow a false positive with an optimistic check. Also, since undefined is not in json standard, the existence becomes ephemeral in languages where that’s distinct from just being undefined. You may think that returning undefined from a wrapped worker is okay, but it results in “error encountered: undefined” down the line due to {} payload. It’s one of the things that make programming harder for no good reason.

Why including that in the response body? 200 status code states everything.

Reply on news.ycombinator.com