gofiber upgrade to v2.51.0 and application/problem+json update

[richardrdev]

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.

[bfabio]

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.

Looks perfect, thanks!

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.

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 validationFunc, but it's way more boilerplate. We kinda experimented a different approach in https://github.com/italia/developers-italia-api/issues/91, but I don't see it as a huge improvement.

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.

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.

It's perfectly fine, as that's the version introducing application/problem+json. It keeps this PR atomic.

Our extensive tests + the semantic versioning of gofiber should be enough.

PS. the linter workflow should now pass (#228)

[richardrdev]

Okay, checks are passing now, I believe this is good to go.

One note - currently I have the detail property always added to the error response, even if it's empty, while validationErrors is only being added to responses where its non-nil. That's fine and works, but for consistency, would we instead want to always include validationErrors, and just have it be blank when it's not relevant? Would make the end of CustomErrorHandler a bit cleaner, and the tests would be easy to update to match. I can add that before the PR is merged if you like, just let me know. Not super important either way imo, I think it'd just be a bit cleaner.

[bfabio]

Okay, checks are passing now, I believe this is good to go.

One note - currently I have the detail property always added to the error response, even if it's empty, while validationErrors is only being added to responses where its non-nil.

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.

[richardrdev]

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

[bfabio]

Nice job @richardrdev, thanks!