Correct format lookup logic. And a few other things - #1862
Conversation
krowvin
left a comment
krowvin
left a comment
There was a problem hiding this comment.
Appears to be failing tests from this PR and the package lock slipped in again
| } else { | ||
| //If the DTO parameter is null, alias map is empty. Compare against well-known types | ||
| //Only use the ContentType classes initialized in contentTypeList rather than | ||
| //the client headers itself | ||
| ContentType type = new ContentType(ct); | ||
| if (contentTypeList.contains(type)) { | ||
| contentTypes.add(type); | ||
| } |
There was a problem hiding this comment.
From the CI tests this looks like it's causing the existing JSON/XML requests to return 406
i.e.
833 tests completed, 83 failed, 20 skipped
via
https://github.com/USACE/cwms-data-api/actions/runs/30954615142/job/92144620012
BasinControllerIT.test_get_create_delete expected HTTP 200 but received 406
There was a problem hiding this comment.
Yep... that's the problem. The removal of this code is correct, otherwise an invalid type "mapping" gets in below instead of properly failing, so now I'm dealing with the affects of that.
e1b6693 to
d69e54b
Compare
There was a problem hiding this comment.
Thanks for fixing those other concerns.
Just had one comment about the Locaiton.java class.
and
This PR is starting to get large because it also looks like you bumped the DB image, updated location delete error handling, and are tweaking the RSS rate limit test. (You may want to change the PR title if the scope was intended to be larger?)
This might turn into one of those endless soups!
| @FormattableWith(contentType = Formats.XMLV2, formatter = XMLv2.class, aliases = {Formats.XML}) | ||
| @FormattableWith(contentType = Formats.JSONV2, formatter = JsonV2.class, aliases = {Formats.DEFAULT, Formats.JSON}) | ||
| @FormattableWith(contentType = Formats.JSONV1, formatter = JsonV1.class) | ||
| @FormattableWith(contentType = Formats.GEOJSON, formatter = JsonV2.class) |
There was a problem hiding this comment.
Is this going to apply to every endpoint of Location.class (not just the getAll)?
If it makes a GET /locations/{id} accept geo+json and serializes JSON v2, it will label it GeoJSON no?
Might need a get all specific marker instead?
There was a problem hiding this comment.
Because later the normal formatter gets called here
There was a problem hiding this comment.
Hmm, yeah, that one should probably be the DUMMY formatter.
But to be clear the format mapping is per dto/content-type an end point may further constrain, or redirect if it needs to.
Thankfully, this is the end. I could've waited on the RSS limit but it was just bugging the crap out of me. The db image was a simple one, finally got release so needed an update. That said, thanks for pushing back on scope keep, it generally is a bad idea. |
MikeNeilson
changed the title
…tentTypeAliasMap.
ecfc930 to
caf7286
Compare
2 participants
Summary
Process the provided header in such a way as to not accidentally attempt to use an incorrect content type.
primary fix, always map the primary content type to itself.
Remove the logic in parseHeader that allow creating an unmapped ContentType object.
Then add appropriate annotations to deal with the fallout of doing that.
Related Issue
Closes #1861
Validation
Additional test for Formats, existing tests.
Checklist