Add example to fizz.Response & add fizz.ResponseWithExamples

[nikicc]

It's me again with proposed changes for handling examples 🙈

NOTICE: This PR is far from being done, but I just wanted to reach out and check if this is something we want to put in eventually before I spend more time polishing it. Let me know what you think.

Idea

With these changes, we could provide one or more examples with the newly proposed OperationOptions. The core of the idea is that, even if the response structs for some endpoints (or responses) are the same, we might want to show different examples.

For example in my codebase, we use the Error struct (see below) as a generic struct for returning all the server errors. And while the same struct is used on responses with codes 400, 404, and 500, we would like examples to differ; which is not possible with the newly introduced example tags (#43).

Shortly, the PR introduces two more operation information, allowing to set examples:

• fizz.ResponseWithExample: extends fizz.Response and overwrite a single example,
• fizz.ResponseWithExamples: extends fizz.Response and overwrite multiple examples.

Example codebase:

package main

import (
    "fmt"
    "net/http"

    "github.com/gin-gonic/gin"
    "github.com/loopfz/gadgeto/tonic"
    "github.com/wI2L/fizz"
)

type Error struct {
    Code    string `json:"code" example:"example-value-from-struct"`
    Message string `json:"message" example:"example-value-from-struct"`
}

var (
    ErrorBadRequest1 = Error{"001", "bad request 1"}
    ErrorBadRequest2 = Error{"002", "bad request 2"}
    ErrorBadRequest3 = Error{"003", "bad request 3"}
    ErrorInternal    = Error{"999", "internal server"}
)

func handler(c *gin.Context) error {
    return nil
}

func main() {
    f := fizz.NewFromEngine(gin.New())
    f.GET("/openapi", nil, f.OpenAPI(nil, "json"))

    f.POST(
        "/create",
        []fizz.OperationOption{
            // case without the example: example value taken from struct if exists
            fizz.Response("404", "Bad request", Error{}, nil),
            // case of passing a single example
            fizz.ResponseWithExample("500", "Internal server error", Error{}, nil, ErrorInternal),
            // case of passing multiple examples
            fizz.ResponseWithExamples("400", "Bad request", Error{}, nil, map[string]interface{}{
                "bad1": ErrorBadRequest1,
                "bad2": ErrorBadRequest2,
                "bad3": ErrorBadRequest3,
            }),
        },
        tonic.Handler(
            handler,
            201,
        ),
    )

    fmt.Println("Errors:", f.Errors())

    srv := &http.Server{Addr: ":4242", Handler: f}
    srv.ListenAndServe()
}

[codecov[bot]]

Codecov Report

Merging #45 (8aee5a4) into master (ffa3de6) will increase coverage by 0.08%. The diff coverage is 95.65%.

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   96.09%   96.17%   +0.08%     
==========================================
  Files           7        7              
  Lines         870      889      +19     
==========================================
+ Hits          836      855      +19     
  Misses         20       20              
  Partials       14       14              

Impacted Files	Coverage Δ
openapi/generator.go	94.76% <92.30%> (+0.08%)	:arrow_up:
fizz.go	98.58% <100.00%> (+0.09%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ffa3de6...8aee5a4. Read the comment docs.

[wI2L]

Yes, that's something that would be a great addition. Everything that matches the OpenAPI spec is welcoms usually.

I read the first implementation quicky, I'll provide a pre-review with my thoughts tomorrow.

[nikicc]

@wI2L happy to hear 👍

I had some extra time today so I added the tests and docs. I think this is now ready for a proper review.

[wI2L]

@nikicc

I read the code, and before I go on with the implementation details, I'd like to tak a break and talk about the new API functions that are introduced by this PR ResponseWithExample and ResponseWithExamples. The OpenAPI spec stipulates the following:

The example field is mutually exclusive of the examples field

This means that the combined usage of the functions mentionned before is invalid and should be mentionned.

Furthermore, the newly introduced function ResponseWithExample should be removed in favor of updating the already existing Response function. We juste need to add the example parameter at the end of the prototype (a nil example would be ignored). Remember that we're pre-v1, so we can introduce breaking changes.

So we would end up with two funcs:

• Response
• ResponseWithExamples

that I think are easier to read.

What do you think ?

[nikicc]

@wI2L I like it and I agree about both points 👍

I think having just two funcs is definitely nicer, but I wasn't sure about the strategy for introducing breaking changes (and hence opted for the safer choice). But I'm happy to do a breaking change to Response if you are.

As for the mutual exclusiveness: yes, we should mention this. I'll add a note to the docs.

I'll do these changes now and push shortly.

[wI2L]

@nikicc Regarding the mutually exclusive usage of Example and Examples in the spec, I think not only do we need to mention it, but actually do something in the code to prevent filling in both fields if both functions are used. Deciding with case (single example vs many) has precedence is tricky, but we could simply assume that the first function evaluated has precedence, and further calls that attempt to modify the "other" field would be denied (note that several calls to fizz.Reponse should override previous, in therory).

[nikicc]

@wI2L, in theory, I totally agree. But in this specific case, I don't think it's that problematic. Mainly, as I don't see a way for a user to set both with the API we are currently exposing.

What was the case you had in mind that worries you? Is it calling Response and ResponseWithExample like in this case:

[]fizz.OperationOption{
    fizz.Summary("Remove a fruit from the market"),
    fizz.Response("400", "Bad request", nil, nil,
        map[string]interface{}{"error": "fruit already exists"},
    ),
    fizz.ResponseWithExamples("400", "Bad request", nil, nil, map[string]interface{}{
        "fruitNotFound": map[string]interface{}{"error": "fruit not found"},
        "invalidApiKey": map[string]interface{}{"error": "invalid api key"},
    }
}

If so, the above example currently crashes either way with response with code 400 already exists. So if we keep this rule that prevents overwriting responses for the same error code, I don't see a way of how one could set both the example and examples. Or am I missing something here?

So I think this:

further calls that attempt to modify the "other" field would be denied

is already implemented, as we do not allow the updates to the error codes.

Further, even if we ever change that rule, and say that overwriting is a valid thing, then IMO we should purge whatever was there before (along with either example or examples) and construct the whole response from scratch with whatever was given in the second call. I don't think we need to handle partial updates, which is the only case I see where these things could become messy.

But to be safe, I propose we do a check in the setOperationResponse function and raise an error (or even crash) in the case when both example & examples are provided. Also, I don't think we need to decide whether example or examples gets preference, since I'm not sure it can happen in the current codebase. Since this is the only place where we set the Example and Examples to the MediaType resource, with this check, we should be safe if in the future that method is called with both example & examples.

Let me know what do you think. Would a check setOperationResponse be sufficient, or do you think we need more?

[wI2L]

@nikicc

You are right, I forgot about the fact that the list of responses of an operation is indexed by the status code, which must be unique. I was re-reading the spec about examples, and it aligns with what you described.

So, since setOperationResponse already checks that a response is unique and the method cannot be called with examples and examplesset, this is fine.

[nikicc]

@wI2L yes, I think it should be good. However, I added an explicit check that both example and examples cannot be set when calling setOperationResponse ... just to be super safe.

From my side, this is now all ready for review, let me know what you think about the code 😊

[wI2L]

@nikicc Can you please squash the commits ? EDIT: and fix the conflict in testdata, I just merged another feature branch, you might need to rebase.

[nikicc]

@wI2L will rebase now. About commits, do you want them squashed? One is a feature implementation, other is an unrelated typo fix. I personally would prefer them to be separate but can squash if you want.

[wI2L]

@nikicc Ah nvm then, I didn't realized you fixed the Operation typo in a separate commit.

[wI2L]

@nikicc Sorry for the delay today, been busy and travis took ages to run testbuild. I'm going to release another minor tag that include the changes. Thanks for your second contribution.

[nikicc]

@wI2L no worries. This still went through quickly 😊 Also, thanks for making a release 🚀