Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve responses when http request validation fails #2649

Open
mistermoe opened this issue Sep 11, 2024 · 1 comment
Open

Improve responses when http request validation fails #2649

mistermoe opened this issue Sep 11, 2024 · 1 comment
Labels

Comments

@mistermoe
Copy link
Collaborator

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:

  • what is lnurl.GetPayReqRequest?
  • is this the expected shape of some json object i'm supposed to send as a request body?

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

@github-actions github-actions bot added the triage Issue needs triaging label Sep 11, 2024
@mistermoe
Copy link
Collaborator Author

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]
}

@worstell worstell added the dx label Sep 11, 2024
This was referenced Sep 11, 2024
@gak gak added P2 and removed triage Issue needs triaging labels Sep 11, 2024
@wesbillman wesbillman assigned wesbillman and unassigned wesbillman Sep 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants