Skip to content

FIX: Handle JSON markdown format exceptions #435

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

Merged
merged 3 commits into from
Oct 8, 2024

Conversation

meisman-ms
Copy link
Contributor

Description

This PR handles variations of markdown formatted JSON responses which were not previously handled, resulting in exceptions (see #413 for details).

I decided to create a few helper functions in order to maintain code clarity while providing a more robust and extensible solution. Should new or additional patterns need handled in the future, it's likely that modifying the regex to include the new pattern will be sufficient without necessitating logic modifications.

Technical Details

Originally, only one specific case was handled, so if the markdown syntax was not in this format ```json\n .... \n```, it would result in an exception. Now the handler can resolve any combination of the following start and end notations, as well as scenarios where only the start or end markdown was present.

Start Notations Handled: ```json\n, `json\n, ```\n, `\n, ```json, `json, ```, `, json, json\n
End Notations Handled: \n```, \n`, ```, `

Finally, after removing any markdown, I added validation for proper JSON format. If the format is still invalid, another attempt will be taken to extract any valid JSON object or array. This could be useful in many cases; for instance, if the starting markdown tag had a typo such as ```jsn\n, the helper function which removes the starting tag would only remove the ```, so it would still be improper JSON without this final check.

Tests and Documentation

I added unit tests for the new functions I created, as well as for the original exception handler function which was not previously tested. I added comments for each function to document the purpose and usage.

Code Coverage

Coverage Before ### Combined Coverage:
CoverageAfterOverall
### File Specific Coverage:
PreviousCoverage ### Uncovered lines (related to this work item):
PreviousCoverageDetails
Coverage After ### Combined Coverage:
CoverageAfterOverall
### File Specific Coverage:
CoverageAfter
### New File:
CoverageDetails_helpers
### Previously Untested:
CoverageDetails_classes

Validation

  • All unit tests are passing
  • Code coverage after >= before
  • Pre-commit hooks / lint passed

@romanlutz @nina-msft

Copy link
Contributor

@romanlutz romanlutz left a comment

Choose a reason for hiding this comment

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

Really cool! This is a lot more sophisticated than what I had intended and doesn't even add complexity. Thank you for this contribution.

@rlundeen2
Copy link
Contributor

This looks good to me; after addressing Roman's comments if build passes I am happy to merge this. Great work @meisman-ms!!!

@meisman-ms meisman-ms force-pushed the meisman/handle-json-md-variants branch from ca2bae2 to a28b41a Compare October 7, 2024 17:21
@meisman-ms
Copy link
Contributor Author

Thanks @romanlutz for the feedback, I believe my last commit addresses both items. And thank you @rlundeen2 as well for the review :)

@rlundeen2 rlundeen2 merged commit b6976a8 into Azure:main Oct 8, 2024
5 checks passed
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.

BUG responses that start with markdown code indicator but don't end with it are not cleaned up
3 participants