Skip to content

Conversation

@aldur
Copy link
Contributor

@aldur aldur commented Aug 11, 2021

The following would always raise an exception in the try block and raise the second exception from the except without parsing the JSON message received in the HTTP response.

            try:
                 raise error.AlgodHTTPError(json.loads(e)["message"], code)
            except:
                 raise error.AlgodHTTPError(e, code)

If the intent was to decode the JSON in the exception, this PR fixes the behaviour. Before:

{"message":"TransactionPool.Remember: transaction CON6STFUWXIL3JHMHQOTSM5753NKOJKONGUVTD43DIG2P2ZRCMMA: asset 110 frozen in GSRKKDAQNOKAL4JJDJJLJUB3ZJFKTRI4NHXZ4QHYYPXSOE3EEWSWB43PRM"}

After:

TransactionPool.Remember: transaction CON6STFUWXIL3JHMHQOTSM5753NKOJKONGUVTD43DIG2P2ZRCMMA: asset 110 frozen in GSRKKDAQNOKAL4JJDJJLJUB3ZJFKTRI4NHXZ4QHYYPXSOE3EEWSWB43PRM

I have added a small integration test, but I can't run the Docker tests because indexer fails to build (locally and on CI).

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

I agree looks as a fix.

@aldur
Copy link
Contributor Author

aldur commented Aug 11, 2021

👍, I'll try to run (and fix if needed) unit tests before merging.

@algochoi
Copy link
Contributor

Hey @aldur, I think the indexer build issues have been resolved by this PR. Thanks for your patience :)

@algorandskiy
Copy link
Contributor

algorandskiy commented Aug 13, 2021

Failed with

FAILURE in step 'we make any "algod" call to "any".':
  Feature:  REST Client Responses
  Scenario: algod: any - algod_applications_ErrorResponse_0.json -- @1.26 
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/behave/model.py", line 1806, in run
    match.run(runner.context)
  File "/usr/local/lib/python3.7/site-packages/behave/matchers.py", line 97, in run
    self.func(context, *args, **kwargs)
  File "test/steps/v2_steps.py", line 99, in step_impl
    validate_error(context, err)
  File "test/steps/v2_steps.py", line 80, in validate_error
    assert context.expected_mock_response == json.loads(err.args[0])
  File "/usr/local/lib/python3.7/json/__init__.py", line 348, in loads
    return _default_decoder.decode(s)
  File "/usr/local/lib/python3.7/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/usr/local/lib/python3.7/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)

Please double check if it related to the fix somehow.

The code

def validate_error(context, err):
    if context.expected_status_code != 200:
        if context.expected_status_code == 500:
            assert context.expected_mock_response == json.loads(err.args[0])                  # <--- !!! HERE !!!
        else:
            raise NotImplementedError("test does not know how to validate status code " + context.expected_status_code)
    else:
        raise err


@when('we make any "{client}" call to "{endpoint}".')
def step_impl(context, client, endpoint):
    # with the current implementation of mock responses, there is no need to do an 'endpoint' lookup
    if client == "indexer":
        try:
            context.response = context.icl.health()
        except error.IndexerHTTPError as err:
            validate_error(context, err)
    elif client == "algod":
        try:
            context.response = context.acl.status()
        except error.AlgodHTTPError as err:
            validate_error(context, err)                      # <--- !!! HERE !!!
    else:
        raise NotImplementedError('did not recognize client "' + client + '"')

@aldur
Copy link
Contributor Author

aldur commented Aug 16, 2021

Thank you @algochoi!

@algorandskiy, fixed the same issue in the indexer and adapted integration tests to match. All green from my side!

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you for fixing this

@jasonpaulos jasonpaulos merged commit 5bbfe84 into develop Aug 16, 2021
aldur added a commit that referenced this pull request Feb 8, 2022
* Fix JSON decoding in AlgodHTTPError

* Add logging around error

* Apply same fix to indexer

* Fix integration tests
@algorandskiy algorandskiy deleted the aldur/fix_exception_message branch February 18, 2022 20:59
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.

5 participants