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

Add robustness for missing TupleComponents #1194

Merged

Conversation

mikaylathompson
Copy link
Collaborator

@mikaylathompson mikaylathompson commented Dec 11, 2024

Description

While prepping for the demo today, we encountered a bug while checking the tuples. A tuple was throwing the following error (shown with a minimal-ish valid tuple):

(.venv) sh-5.2# echo '{"sourceRequest":{"Request-URI":"/_snapshot/migration_assistant_repo/rfs-snapshot","Method":"DELETE","HTTP-Version":"HTTP/1.1","payload":{"inlinedTextBody":""}},"targetRequest":{"Request-URI":"/_snapshot/migration_assistant_repo/rfs-snapshot","Method":"DELETE","payload":{"inlinedTextBody":""}},"targetResponses":[{"Date":["Tue, 10 Dec 2024 21:07:03 GMT"],"Content-Type":["application/json; charset=UTF-8"],"Status-Code":404,"Reason-Phrase":"Not Found","response_time_ms":49,"payload":{"inlinedJsonBody":{}}}],"connectionId":"024ad2fffe460aed-00000007-00000ac1-61aca27669436ce2-fdb9a98a.1563","numRequests":4,"numErrors":0}' | console tuples show
Traceback (most recent call last):
...
  File "/root/lib/console_link/console_link/middleware/tuples.py", line 10, in convert
    tuple_reader.transform_stream(inputfile, ouptutfile)
  File "/root/lib/console_link/console_link/models/tuple_reader.py", line 26, in transform_stream
    json.dump(next(transformer), outputfile)
              ^^^^^^^^^^^^^^^^^
  File "/root/lib/console_link/console_link/models/tuple_reader.py", line 34, in _transform_lines
    yield parse_tuple(line, i + 1)
          ^^^^^^^^^^^^^^^^^^^^^^^^
  File "/root/lib/console_link/console_link/models/tuple_reader.py", line 194, in parse_tuple
    tuple_component = TupleComponent(component, initial_tuple[component], line_no, is_bulk_path)
                                                ~~~~~~~~~~~~~^^^^^^^^^^^
KeyError: 'sourceResponse'

This tuple is as-expected, except that it doesn't have a sourceResponse field, which is a valid state for a tuple. The correct behavior is to translate all the present components of the tuple, but not to worry about the missing ones.

The fixed behavior (same tuple):

(.venv) sh-5.2# echo '{"sourceRequest":{"Request-URI":"/_snapshot/migration_assistant_repo/rfs-snapshot","Method":"DELETE","HTTP-Version":"HTTP/1.1","payload":{"inlinedTextBody":""}},"targetRequest":{"Request-URI":"/_snapshot/migration_assistant_repo/rfs-snapshot","Method":"DELETE","payload":{"inlinedTextBody":""}},"targetResponses":[{"Date":["Tue, 10 Dec 2024 21:07:03 GMT"],"Content-Type":["application/json; charset=UTF-8"],"Status-Code":404,"Reason-Phrase":"Not Found","response_time_ms":49,"payload":{"inlinedJsonBody":{}}}],"connectionId":"024ad2fffe460aed-00000007-00000ac1-61aca27669436ce2-fdb9a98a.1563","numRequests":4,"numErrors":0}' | console tuples show
{"sourceRequest": {"Request-URI": "/_snapshot/migration_assistant_repo/rfs-snapshot", "Method": "DELETE", "HTTP-Version": "HTTP/1.1", "payload": {"inlinedTextBody": ""}, "body": {}}, "targetRequest": {"Request-URI": "/_snapshot/migration_assistant_repo/rfs-snapshot", "Method": "DELETE", "payload": {"inlinedTextBody": ""}, "body": {}}, "targetResponses": [{"Date": ["Tue, 10 Dec 2024 21:07:03 GMT"], "Content-Type": ["application/json; charset=UTF-8"], "Status-Code": 404, "Reason-Phrase": "Not Found", "response_time_ms": 49, "payload": {"inlinedJsonBody": {}}, "body": {}}], "connectionId": "024ad2fffe460aed-00000007-00000ac1-61aca27669436ce2-fdb9a98a.1563", "numRequests": 4, "numErrors": 0}

I add a log message at info level, so a user can add -v to view these logs (and others), but they don't indicate an error and shouldn't be surfaced to most users.

Issues Resolved

Testing

Manual, working on a unit test.

Check List

  • New functionality includes testing
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Collaborator

@lewijacn lewijacn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

codecov bot commented Dec 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.61%. Comparing base (6d3b225) to head (8d8799a).
Report is 28 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1194      +/-   ##
============================================
+ Coverage     80.55%   80.61%   +0.06%     
- Complexity     3074     3080       +6     
============================================
  Files           421      421              
  Lines         15545    15598      +53     
  Branches       1047     1053       +6     
============================================
+ Hits          12522    12575      +53     
  Misses         2380     2380              
  Partials        643      643              
Flag Coverage Δ
unittests 80.61% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@mikaylathompson mikaylathompson merged commit 0a329c0 into opensearch-project:main Dec 11, 2024
22 checks passed
@mikaylathompson mikaylathompson deleted the tuple-display-bugfix branch December 11, 2024 04:37
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