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: catch exceptions due to failure to create XML from text #242

Merged
merged 1 commit into from
Oct 1, 2024

Conversation

joanise
Copy link
Member

@joanise joanise commented Oct 1, 2024

I don't know what causes this, but we got a few 5xx errors from the Heroku logs with this trace:

   File "/app/readalongs/web_api.py", line 209, in assemble
     parsed = parse_xml(
              ^^^^^^^^^^
   File "/app/readalongs/text/util.py", line 138, in parse_xml
     return etree.fromstring(
            ^^^^^^^^^^^^^^^^^
 lxml.etree.XMLSyntaxError: PCDATA invalid Char value 3, line 8, column 26

While it makes no sense we should fail to parse XML we just generated in this broader block:

    parsed = parse_xml(
        create_ras_from_text(
            (request.input_text or "").splitlines(keepends=True),
            text_languages=request.text_languages,
        )
    )

let's just put a try/except in there since we saw it live on Heroku.

Whatever the error in the input is, though I cannot reproduce it, should generate a 422 HTTP code.

PR Goal?

Fix the 5xx errors we saw on Heroku today.

Feedback sought?

sanity check

Priority?

high

Tests added?

can't reproduce, so none

How to test?

beats me, I can't figure out what caused this to happen on Heroku in the first place.

Confidence?

high, because it's a safe patch: there's no risk in adding a try/except block that should never get triggered.

I still wish I could reproduce it, though, and thus unit test it.

Version change?

no

Copy link

semanticdiff-com bot commented Oct 1, 2024

Review changes with SemanticDiff.

Analyzed 1 of 1 files.

Overall, the semantic diff is 62% smaller than the GitHub diff.

Filename Status
✔️ readalongs/web_api.py 61.79% smaller

@joanise joanise requested a review from roedoejet October 1, 2024 20:30
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.90%. Comparing base (791e0b9) to head (1eea719).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
readalongs/web_api.py 50.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
+ Coverage   87.21%   87.90%   +0.69%     
==========================================
  Files          21       21              
  Lines        1783     1786       +3     
  Branches      323      323              
==========================================
+ Hits         1555     1570      +15     
+ Misses        189      179      -10     
+ Partials       39       37       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@roedoejet roedoejet left a comment

Choose a reason for hiding this comment

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

looks good. Maybe ask the user to send us the text?

@joanise
Copy link
Member Author

joanise commented Oct 1, 2024

I guess it's unlikely the user would see the actual text of the error message, but I guess it would not hurt.

I don't know what causes this, but we got a few 5xx errors from the
Heroku logs with this trace:
```
   File "/app/readalongs/web_api.py", line 209, in assemble
     parsed = parse_xml(
              ^^^^^^^^^^
   File "/app/readalongs/text/util.py", line 138, in parse_xml
     return etree.fromstring(
            ^^^^^^^^^^^^^^^^^
 lxml.etree.XMLSyntaxError: PCDATA invalid Char value 3, line 8, column 26
```

While it makes no sense we should fail to parse XML we just generated
in this broader block:
```
    parsed = parse_xml(
        create_ras_from_text(
            (request.input_text or "").splitlines(keepends=True),
            text_languages=request.text_languages,
        )
    )
```
let's just put a try/except in there since we saw it live on Heroku.

Whatever the error in the input is, though I cannot reproduce it, should
generate a 422 HTTP code.
@joanise joanise merged commit 3cab240 into main Oct 1, 2024
5 of 6 checks passed
@joanise joanise deleted the dev.ej/fix-5xx branch October 1, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants