gofiber upgrade to v2.51.0 and application/problem+json update #226
Conversation
Looks perfect, thanks!
Yeah, this is painful and a shortcoming of our tests. Ideally we should have a helper method in tests that transforms the expected string and the result into something comparable, despite the order of the fields - preferably with a pretty printed output of the differences. I don't know if there's already a library doing that or we need do write it ourselves. There's also Maybe we can adapt the strings for this PR as a short term solution, but work on that helper function as a long term one.
It's perfectly fine, as that's the version introducing Our extensive tests + the semantic versioning of PS. the linter workflow should now pass (#228) |
|
Okay, checks are passing now, I believe this is good to go. One note - currently I have the |
internal/common/errors.go
Outdated
| if problemJSON.ValidationErrors != nil { | ||
| err = ctx.JSON(fiber.Map{ | ||
| "title": problemJSON.Title, | ||
| "detail": problemJSON.Detail, | ||
| "status": problemJSON.Status, | ||
| "validationErrors": problemJSON.ValidationErrors, | ||
| }, "application/problem+json") | ||
| } else { | ||
| err = ctx.JSON(fiber.Map{ | ||
| "title": problemJSON.Title, | ||
| "detail": problemJSON.Detail, | ||
| "status": problemJSON.Status, | ||
| }, "application/problem+json") | ||
| } | ||
|
|
||
| return err |
There was a problem hiding this comment.
I think we can simplify this and have JSON be serialized with the rules specified in ProblemJSON's struct fields, as we already have those: https://github.com/italia/developers-italia-api/blob/main/internal/common/problem_json.go#L7-L13
Something like:
| if problemJSON.ValidationErrors != nil { | |
| err = ctx.JSON(fiber.Map{ | |
| "title": problemJSON.Title, | |
| "detail": problemJSON.Detail, | |
| "status": problemJSON.Status, | |
| "validationErrors": problemJSON.ValidationErrors, | |
| }, "application/problem+json") | |
| } else { | |
| err = ctx.JSON(fiber.Map{ | |
| "title": problemJSON.Title, | |
| "detail": problemJSON.Detail, | |
| "status": problemJSON.Status, | |
| }, "application/problem+json") | |
| } | |
| return err | |
| return ctx.JSON(problemJSON, "application/problem+json") |
I think not having the property (ie. setting it to nil) better reflects what we want to convey: there's no data for that field. |
|
Ah, thanks for linking the struct definition Fabio, I didn't realize the values had omitempty. That makes this much easier. Changed most of the test expectedBody values back. this method seems to use the original ordering. Will probably take a look at improving the expectedBody test matching as discussed. Anyway, I believe this is done now, thanks for the help. Fixes issue #225 |
|
Nice job @richardrdev, thanks! |
Not actually ready to merge yet, due to failing tests (see below)
I believe this is right, but I wanted to check that this is looking right to y'all.
Response looks good, identical to response before change, except that the order of properties is moved around.
Because of the changed order in the response, many tests will need to be changed. For example in TestApi:
expectedBody: `{"title":"Not Found","detail":"Cannot GET /v1/i-dont-exist","status":404}`,will need to become
expectedBody: `{"detail":"Cannot GET /v1/i-dont-exist","status":404,"title":"Not Found"}`,and so on. Lots of little manual changes in the expected body values. I'll also need to add a check for empty details, because some tests are failing due to
details: ""being included in the response.I can do those manual changes in the tests, but it'll take some time so I'll need to come back to it later today or tomorrow.
Also, I went ahead and upgraded gofiber to v2.51. Doesn't seem to introduce any breaking changes, so I assume that's fine? I didn't check the gofiber upgrade very thoroughly; I would understand if that needs to be a separate PR though.