tamasfe/aide

Path summary combines docs for each method

dnut opened this issue · 6 comments

dnut commented

I would expect this to document the post and get requests separately:

ApiRouter::new()
    .api_route_with("/files", post(upload_file), |x| x.summary("Upload a file"))
    .api_route_with("/files", get(list_files), |x| x.summary("Get a list of files."))

Instead, they get concatenated into a single summary at the URI level, delimited by \n

"/files": {
  "summary": "Upload a file.\nGet a list of files.",
  "get": {
    "parameters": [
    ... (no summary)

These docs are pretty outdated, but it's the first thing I could find: https://swagger.io/docs/specification/2-0/describing-responses/#response-that-returns-a-file

Anyway, they show that you can include a summary for each method. I'm guessing the same applies to OpenAPI 3.1

  /ping:
    get:
      summary: Checks if the server is alive.
      responses:

When I have some more free time, I can dig into the 3.1 docs, and I can write a complete reproducing example. Let me know if that would be helpful.

dnut commented

I spent some more time looking at the code, and figured out how to do what I wanted:

.api_route_with("/files", post(upload_file), |mut x| {
        let _ = TransformCallback::new(x.inner_mut())
            .post(|x| x.summary("Upload a file."));
        x
    })

This is a very ugly way to get what I would consider the normal expected behavior. The call to api_route_with is specific to a (path, method) pair, so the docs exposed in that call should be at the same point in the hierarchy. It feels like a bug to expose a different level of the hierarchy.

I realize that if it was changed to the way I want it, then it leaves a question of how to document the path. It's probably easier to navigate down the hierarchy than up it. Perhaps there should be a separate method call for that, maybe ApiRouter::transform_path.

The call to api_route_with is specific to a (path, method) pair

It is not, you can write api_route_with("/", get(download_file).post(upload_file), ...), so api_route_with transforms the path item itself.

To modify the operations, you can use get_with (post_with, ...):

.api_route(
    "/files",
    get_with(download_file, |op| op.summary("download a file"))
        .post_with(upload_file, |op| op.summary("upload a file")),
)
dnut commented

That's great, thanks for the help. I never realized you could chain those calls in axum. The code is fine then, I just didn't understand it.

I now see get_with and post_with are described in the "Responses" section of aide::axum. I actually saw this section before writing any code. I'm trying to backtrack my workflow to figure out how the docs could be improved to prevent this confusion, but I think it was just a problem with me. The docs are clear.

I guess one thing I don't see in the docs is the ability to chain operations. But you don't need that information to understand that you can document the operations directly. And the chaining is an axum feature that is probably described in the axum docs already.

dnut commented

Switching to get_with and post_with wiped out all my response types from the schema. This was unexpected and seemed like a bug at first. All I wanted to do was add a summary. I prefer to infer the responses from the types.

I looked at the code and realized it's because they check infer_responses, whereas the basic get and post infer regardless without checking. I had to call infer_responses(true); to modify the global state to enable it.

I can see a justification for this behavior, but it was still unexpected. Maybe this should be added to the docs. Maybe it would be best to infer the response types unless the user explicitly sets them manually.

Wicpar commented

indeed, imo the default should be that get() and get_with() by default use the inferred response. I have the same issue in some of my endpoints, but don't have the time yet to document it further.

It looks indeed like a bug,get and get_with should have identical behaviour.

Currently they are copy/pasted implementations that seems to have got out of sync. The fix would be to refactor the bare functions so that they just call the _with ones with a transformer fn that does nothing.