Correct format lookup logic. Always map primary type to itself in Con… - #1862
Correct format lookup logic. Always map primary type to itself in Con…#1862MikeNeilson wants to merge 11 commits into
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
There was a problem hiding this comment.
I would remove this from this PR
There was a problem hiding this comment.
yeah, that one keeps sneaking in.
There was a problem hiding this comment.
This looks like it's trying to accommodate the change for
https://github.com/USACE/cwms-data-api/pull/1862/changes#r3721590710
Should we just revert this and undo that other change. Just thinking it might be out of scope for the header fix
There was a problem hiding this comment.
I might, see below.
| } 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.
| .jsonConfig( | ||
| JsonConfig.jsonConfig() | ||
| .numberReturnType(JsonPathConfig.NumberReturnType.DOUBLE)); | ||
| .numberReturnType(JsonPathConfig.NumberReturnType.DOUBLE)) |
There was a problem hiding this comment.
If we set all JSON numbers to double won't this break many of the existing integration tests?
There was a problem hiding this comment.
It shouldn't as this was always supposed to be set, and we were setting it manually in the time series tests.
I might change it back though, at least for the numbers, the real change is the adding of the content type processing, only just realized we needed to actually assign the new config to the global, so it wasn't properly active before.
| import cwms.cda.formatters.xml.XMLv2; | ||
|
|
||
| @FormattableWith(contentType = Formats.XMLV2, formatter = XMLv2.class, aliases = {Formats.DEFAULT, Formats.XML}) | ||
| @FormattableWith(contentType = Formats.JSONV2, formatter = XMLv2.class, aliases = {Formats.JSON}) |
There was a problem hiding this comment.
| @FormattableWith(contentType = Formats.JSONV2, formatter = XMLv2.class, aliases = {Formats.JSON}) | |
| @FormattableWith(contentType = Formats.JSONV2, formatter = JsonV2.class, aliases = {Formats.JSON}) |
There was a problem hiding this comment.
Intentional, the processing actually bypasses the formatter lookup... though I should add a comment about it.
There was a problem hiding this comment.
have changed it to a "DUMMY" outputformatter so the intentional is more clear.
49fc7e9 to
bd3cb26
Compare
…tentTypeAliasMap.
e1b6693 to
d69e54b
Compare
2 participants
…tentTypeAliasMap.
Summary
Process the provided header in such a way as to not accidentally attempt to use an incorrect content type
Related Issue
Closes #1861
Validation
Additional test for Formats, existing tests.
Checklist