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

Fix Response Status Code for Failing XML Emits #2217

Merged
merged 1 commit into from
Nov 27, 2024

Conversation

allentiak
Copy link
Member

@allentiak allentiak commented Nov 25, 2024

In case that the generation of XML for responses failed, an OperationOutcome was already produced - but the status code was kept at 200. This was incorrect: in such a case that the server is unable to generate the XML output, the status code should be changed to 500.

@allentiak allentiak marked this pull request as draft November 25, 2024 09:59
@allentiak allentiak force-pushed the fix-failing-xml-emit branch 3 times, most recently from 4612f6c to aa5fcfe Compare November 25, 2024 12:09
@allentiak allentiak changed the title Fix "failing XML emit" test Fix: "failing XML emit" Nov 25, 2024
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.96%. Comparing base (8ff3e98) to head (bd5739f).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2217      +/-   ##
==========================================
- Coverage   94.96%   94.96%   -0.01%     
==========================================
  Files         330      330              
  Lines       20252    20258       +6     
  Branches      483      482       -1     
==========================================
+ Hits        19232    19237       +5     
- Misses        538      539       +1     
  Partials      482      482              

see 2 files with indirect coverage changes

@allentiak allentiak changed the title Fix: "failing XML emit" Fix: "invalid XML" logic Nov 25, 2024
@allentiak allentiak marked this pull request as ready for review November 25, 2024 12:25
allentiak added a commit that referenced this pull request Nov 25, 2024
Update both the request body and status in case of errors, not just the
body.
Copy link
Member

@alexanderkiel alexanderkiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your solution is functionally perfect. It's only the naming I have to criticise.

modules/rest-util/src/blaze/middleware/fhir/output.clj Outdated Show resolved Hide resolved
modules/rest-util/src/blaze/middleware/fhir/output.clj Outdated Show resolved Hide resolved
@allentiak allentiak changed the title Fix: "invalid XML" logic Fix "failing XML emit" logic Nov 27, 2024
Copy link
Member

@alexanderkiel alexanderkiel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you please could change the commit message into something like this:

Fix Response Status Code for Failing XML Emits

In case the generation of XML for responses failed, an OperationOutcome was
already produced, but the status code was kept at 200. However in such a
case that the server was unable to generate the XML output, the status
code has to be 500.

I would be perfect.

In case that the generation of XML for responses failed, an
OperationOutcome was already produced - but the status code was kept at
200. This was incorrect: in such a case that the server is unable to
generate the XML output, the status code should be changed to 500.
@allentiak
Copy link
Member Author

Thanks for the suggestion.
I have changed the commit message accordingly.

@allentiak allentiak changed the title Fix "failing XML emit" logic Fix Response Status Code for Failing XML Emits Nov 27, 2024
@alexanderkiel alexanderkiel added this pull request to the merge queue Nov 27, 2024
@alexanderkiel alexanderkiel added bug Something isn't working module:rest-util labels Nov 27, 2024
@alexanderkiel alexanderkiel self-assigned this Nov 27, 2024
@alexanderkiel alexanderkiel added this to the v0.31.0 milestone Nov 27, 2024
Merged via the queue into main with commit d558776 Nov 27, 2024
133 checks passed
@alexanderkiel alexanderkiel deleted the fix-failing-xml-emit branch November 27, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module:rest-util
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants