Should dropshot endpoints be able to return Option<T>?

[jgallagher]

Today, these two endpoints:

#[endpoint {
    method = GET,
    path = "/foo",
}]
async fn get_foo(_rqctx: RequestContext<()>) -> Result<HttpResponseOk<Foo>, HttpError> { .. }

#[endpoint {
    method = GET,
    path = "/option_foo",
}]
async fn get_option_foo(_rqctx: RequestContext<()>) -> Result<HttpResponseOk<Option<Foo>>, HttpError> { .. }

produce identical OpenAPI definitions:

    "/foo": {
      "get": {
        "operationId": "get_foo",
        "responses": {
          "200": {
            "description": "successful operation",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Foo"
                }
              }
            }
          },
          "4XX": {
            "$ref": "#/components/responses/Error"
          },
          "5XX": {
            "$ref": "#/components/responses/Error"
          }
        }
      }
    },
    "/option_foo": {
      "get": {
        "operationId": "get_option_foo",
        "responses": {
          "200": {
            "description": "successful operation",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Foo"
                }
              }
            }
          },
          "4XX": {
            "$ref": "#/components/responses/Error"
          },
          "5XX": {
            "$ref": "#/components/responses/Error"
          }
        }
      }
    }
  },

This is surprising/wrong, and causes generated clients to not realize that get_option_foo() might return None / null instead of a Foo. It's relatively easy to work around this by returning a struct that contains an Option. I don't know if returning an Option directly should work; if not, could we reject it at compile time somehow?

[david-crespo]

I vote for rejecting it. I don't see why we would ever want to return null.

[Nieuwejaar]

I vote for rejecting it. I don't see why we would ever want to return null.

If there is a technical reason this can't or shouldn't be done, that's fine. Given that at least two of us have hit this problem, then "I don't need it, so nobody else should either" doesn't seem like a compelling argument.

[david-crespo]

I didn't realize you were saying you want to be able to return null. In that case I retract 100%. I'm sure we can make the OpenAPI spec reflect the Option. However I believe returning a JSON object containing a nullable field is more idiomatic.

[Nieuwejaar]

This is my specific use case:

async fn link_fault_get(
       rqctx: RequestContext<Arc<Switch>>,
       path: Path<LinkPath>,
   ) -> Result<HttpResponseOk<Option<Fault>>, HttpError> {

We're asking for the details of a fault condition on a link. If the link doesn't have a fault at the moment, I want to return None. You could also make an argument for HTTP 404, but we usually use that to indicate that the requested link itself doesn't exist. We could also add a Fault::None enum, but that would have unfortunate ripple effects throughout the fault processing code.

I'm currently using a workaround suggested by John:

struct FaultCondition {
        fault: Option<Fault>,
}

If it's complicated or undesirable to support HttpResponseOk<Option<foo>> , then this seems like a perfectly reasonable alternative - for my use case at least.

[davepacheco]

I'm not sure whether it makes sense to return null, a struct with a null/optional field, a 404, or even a 204 in this case. But what we're doing now definitely seems wrong in that the spec doesn't reflect what the server can produce.

[ahl]

Agree with the consensus that what we're doing now is wrong.

A maximally accurate approach for an interface that sometimes returns a pile of data and sometimes doesn't would be to support multiple response codes e.g. 200 and 204/404 as @davepacheco suggested above.

I'm not sure how we could generically support multiple success responses. Perhaps it would suffice to have a type like HttpResponseOkOrNoContent<T> that was initialized with an Option<T>. Some would produce a 200; None would produce a 204.

While I like this a lot and thing it might be the best direction, it would necessitate progenitor work--currently we assume a single success response type (and single error response type). This is, however, directionally work I think we should do in progenitor as well.

I suppose there's nothing overtly wrong about a 200 response with content null though we generally prefer a 204 in that case. We could generate a construction like this:

        "responses": {
          "200": {
            "description": "successful operation",
            "content": {
              "application/json": {
                "schema": {
                  "nullable": true,
                  "allOf": [
                    {
                      "$ref": "#/components/schemas/Foo"
                    }
                  ]
                }
              }
            }
          },

That said, I think we'd do better to steer people away from this approach as we've steered people away from the HttpResponseOk(()) response in favor of HttpResponseNoContent.

[andrewjstone]

Just an FYI that I hit this tonight and @gjcolombo pointed me here. For my use case I can use a NotFound. In any case, this was certainly confusing behavior to be confronted with during development. It would be great if the compiler can throw up it's hands in this case.