meraki / dashboard-api-python

Official Dashboard API library (SDK) for Python
MIT License
287 stars 149 forks source link

aio/rest_session.py crashing on 400 errors with a UnboundLocalError #223

Closed dpnetca closed 11 months ago

dpnetca commented 1 year ago

Issue with SDK version 1.36.0 when, I am calling the createOrganizationAdmin, expecting back a 400 bad request error, but script is crashing with below trace. reverting to SDK version 1.34.0 and error is handled properly.

... add_result = await dashboard.organizations.createOrganizationAdmin( ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/xxxxxxxxx/py/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 512, in post async with await self.request(metadata, "POST", url, json=json) as response: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/home/xxxxxxxxxxx/py/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 137, in request return await self._request( ^^^^^^^^^^^^^^^^^^^^ File "/home/xxxxxxxxxxx/py/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 275, in _request message_is_dict

Specifically testing API call in postman and the response back I am getting is: { "errors": [ "Email xxxxxxxx@abcdefg.com is already registered with a Cisco Meraki Dashboard account. For security purposes, that user must verify their email address before administrator permissions can be granted here." ] }

Python version installed 3.11.4

Meraki library version installed 1.36.0 (works properly in 1.34.0 and earlier))

Have you reproduced the issue with the latest version of this library? And with the latest version of Python? yes

OS Platform Linux

How can we replicate the problem you're reporting? Add user to 1 org that does not currently exist, then add user to a second org before they validate and create account, should get back a 400 bad request error as described

ollipa commented 11 months ago

@TKIPisalegacycipher, seems that this was caused by this commit: https://github.com/meraki/dashboard-api-python/commit/6d9ada0df4ad74a4c3f447982c4aa39c9db57433

message_is_dict is used in aio/rest_session but it is never set. In sync code (rest_session) this variable is set as follows but this code block is missing from aio/rest_session:

try:
    message = response.json()
    message_is_dict = True
except ValueError:
    message = response.content[:100]
    message_is_dict = False
TKIPisalegacycipher commented 11 months ago

Hi @dpnetca are you still running into this issue? I've added debug logging here, so if so can you please provide the full debug log on the latest version of the library? Feel free to redact IDs and serial numbers.

dpnetca commented 11 months ago

yes, tested with 1.36.0, 1.37.1 and 1.37.2 and I still get errors, though it looks like it may be slightly different with 3.17.x from 3.16.x

$ pip list | grep meraki
meraki             1.37.2

traceback:

$ python main.py admin.add

Traceback (most recent call last):
  File "/home/xxxxxx/code/py/work/momapy_cli/main.py", line 22, in <module>
    asyncio.run(admin_add())
  File "/home/xxxxxx/.pyenv/versions/3.11.4/lib/python3.11/asyncio/runners.py", line 190, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/.pyenv/versions/3.11.4/lib/python3.11/asyncio/runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/.pyenv/versions/3.11.4/lib/python3.11/asyncio/base_events.py", line 653, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/admin/add.py", line 91, in admin_add
    results = [await task async for task in tqdm_tasks]
              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/admin/add.py", line 91, in <listcomp>
    results = [await task async for task in tqdm_tasks]
               ^^^^^^^^^^
  File "/home/xxxxxx/.pyenv/versions/3.11.4/lib/python3.11/asyncio/tasks.py", line 605, in _wait_for_one
    return f.result()  # May raise f.exception().
           ^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/admin/add.py", line 29, in add_org_admin
    add_result = await dashboard.organizations.createOrganizationAdmin(
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 505, in post
    async with await self.request(metadata, "POST", url, json=json) as response:
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 134, in request
    return await self._request(
           ^^^^^^^^^^^^^^^^^^^^
  File "/home/xxxxxx/code/py/work/momapy_cli/.venv/lib/python3.11/site-packages/meraki/aio/rest_session.py", line 273, in _request
    if message_is_dict and 'errors' in message.keys() \
       ^^^^^^^^^^^^^^^
NameError: name 'message_is_dict' is not defined
dpnetca commented 11 months ago

@TKIPisalegacycipher Difference seems to be here, in the sync rest_session code for 4xx errors the message_is_dict is being initialized to either true or false https://github.com/meraki/dashboard-api-python/blob/86a565890b1ed3e7fd8b8a50e5c4f5df4decda40/meraki/rest_session.py#L270

                    try:
                        message = response.json()
                        message_is_dict = True
                    except ValueError:
                        message = response.content[:100]
                        message_is_dict = False

However, in the equivalent section in the aio library the message_is_dict is not initialized https://github.com/meraki/dashboard-api-python/blob/86a565890b1ed3e7fd8b8a50e5c4f5df4decda40/meraki/aio/rest_session.py#L253

                    try:
                        message = await response.json(content_type = None)
                    except aiohttp.client_exceptions.ContentTypeError:
                        logging.debug(f"message is {message}")
                        logging.debug(f"message is dict? {isinstance(message, dict)}")
                        try:
                            message = (await response.text())[:100]
                            logging.debug(f"message is {message}")
                            logging.debug(f"message is dict? {isinstance(message, dict)}")
                        except:
                            message = None

manually editing my local copy of tat file to the following and code executes without error:

                    try:
                        message = await response.json(content_type=None)
                        message_is_dict = True
                        ...
dpnetca commented 11 months ago

Sorry for the multiple messages, but one more thing of note. looking back at at the 1.36.0 code before the debug was added I can see message_is_dict was initiated here partially, but missing a few things ( see my added comments) the issue is if the first TRY succeeds (which in my case it is) then message_is_dict is not initialized. causing the error. if the first fails and hits the exception, then it is set incorrectly in the second try, in this case it would be a string not a dict but that is still set to True should be False:

                # 4XX errors
                else:
                    try:
                        message = await response.json(content_type = None)
                        # message_is_dict = True should be set here
                    except aiohttp.client_exceptions.ContentTypeError:
                        try:
                            message = (await response.text())[:100]
                            message_is_dict = True   # this should be False not True
                        except:
                            message = None
                            message_is_dict = False
dpnetca commented 11 months ago

quick test and it looks like the issue is resolved with 1.37.3. thank you!!