TBD54566975 / ftl

FTL - Towards a 𝝺-calculus for large-scale systems
https://tbd54566975.github.io/ftl/
Apache License 2.0
21 stars 7 forks source link

Improve responses when http request validation fails #2649

Open mistermoe opened 1 month ago

mistermoe commented 1 month ago

Given the following verb:

type GetPayReqQueryParams struct {
    LNURL string
}

type GetPayReqRequest = builtin.HttpRequest[ftl.Unit, ftl.Unit, GetPayReqQueryParams]

//ftl:typealias
type Decimal = decimal.Decimal

type PayReq struct {
    Callback    string  `json:"callback"`    // The URL from LN SERVICE which will accept the pay request parameters
    MaxSendable Decimal `json:"maxSendable"` // Max millisatoshi amount LN SERVICE is willing to receive
    MinSendable Decimal `json:"minSendable"` // Min millisatoshi amount LN SERVICE is willing to receive. can not be less than 1 or more than `maxSendable`
    Metadata    string  `json:"metadata"`    // Metadata json which must be presented as raw string per LUD06
    Tag         string  `json:"tag"`         // Type of LNURL
}

type LnServiceError struct {
    Status string `json:"status"`
    Reason string `json:"reason"`
}

type GetPayReqResponse = builtin.HttpResponse[PayReq, LnServiceError]

//ftl:ingress GET /lnurl/payreq
func GetPayReq(ctx context.Context, req GetPayReqRequest) (GetPayReqResponse, error) {
    logger := ftl.LoggerFromContext(ctx)

    logger.Infof("lnurl %s", req.Query.LNURL)

    return GetPayReqResponse{
        Status: http.StatusNotImplemented,
        Error:  ftl.Some(LnServiceError{Status: "NotImplemented", Reason: "Not implemented"}),
    }, nil
}

sending a request without the required query param results in the following response:

❯ curl -v "http://localhost:8891/lnurl/payreq"
* Host localhost:8891 was resolved.
* IPv6: ::1
* IPv4: 127.0.0.1
*   Trying [::1]:8891...
* connect to ::1 port 8891 from ::1 port 53779 failed: Connection refused
*   Trying 127.0.0.1:8891...
* Connected to localhost (127.0.0.1) port 8891
> GET /lnurl/payreq HTTP/1.1
> Host: localhost:8891
> User-Agent: curl/8.7.1
> Accept: */*
>
* Request completely sent off
< HTTP/1.1 400 Bad Request
< Content-Type: text/plain; charset=utf-8
< Vary: Origin
< X-Content-Type-Options: nosniff
< Date: Wed, 11 Sep 2024 06:23:42 GMT
< Content-Length: 47
<
lnurl.GetPayReqRequest.query.lnurl is required
* Connection #0 to host localhost left intact

it's awesome that FTL handles validation but i think the response can be improved. Most of the current response is unhelpful and potentially misleading to a caller. If i hadn't implemented this verb and received lnurl.GetPayReqRequest.query.lnurl as a response. i would think:

maybe something along the lines of "lnurl is a required query param"

mistermoe commented 1 month ago

another thought that crossed my mind is that these error messages returned by FTL won't match a developer-defined error response type.

e.g. a developer is designing an API and has decided that all non 2xx responses will have a response body that looks something like:

type APIError struct {
    Message string
    Errors  []Error
}

//ftl:data export
type Error struct {
    Message string
    Code    ftl.Option[int]
    Field   ftl.Option[string]
}