Incompatible Exception usage

[struppinet]

We use Pynetbox in combination with Celery (task queue). Celery does this wonderful thing where it re-creates Exceptions with what it received and therefor we encounter an error called: AttributeError: 'str' object has no attribute 'status_code'

Reason for this is the the RequestError Exception is incompatible with it's base class init. I would suggest to change it so that if just a string is given it still works.

https://github.com/netbox-community/pynetbox/blob/53eaca68829a350333f6da49823a96b9d240e0b5/pynetbox/core/query.py#L43C11-L43C11

actual:

class RequestError(Exception):
...
    def __init__(self, req):
        if req.status_code == 404:

suggestion:

class RequestError(Exception):
...
    def __init__(self, req):
        if isinstance(req, str):
                self.message = req
        elif req.status_code == 404:

[abhi1693]

I do not understand your suggestion and why it would be beneficial to change. Could you provide more details on this? Also, looking at your error message, it seems like you may be calling the message string and then looking for the status_code which could explain the issue.

[struppinet]

No the problem is with the init of the class. The basic exception just takes a string e = Exception('something went wrong') raise e

With the RequestError when the Exception would be re-created freshly (which is my case) e = RequestError('URL does not exist') raise e

Then this fails with AttributeError: 'str' object has no attribute 'status_code' since the init param is just a string and not the expected req dict. Hence my suggestion in checking the init input before assuming it is the expected req.

[markkuleinio]

Why would Celery be raising pynetbox.RequestError exceptions?

I mean, I'd understand it using plain raise (that would raise the original exception) but why would it try to raise a new exception that it doesn't even know how to use it?

There is no requirement in Python that custom exceptions should be accepting a string as the first argument.

[struppinet]

It stores the exceptions for retries and re-raises them. See issue here: https://github.com/celery/celery/issues/3586

Correct, there is no requirement. But it's also not a good practise to be incompatible with the base class. https://en.wikipedia.org/wiki/Liskov_substitution_principle

[markkuleinio]

If RequestError is changed to accept RequestError("some text"), that exception would be incompatible with the current RequestError exceptions.

Is this Celery habit (to raise exceptions it doesn't really know) something that is meant for just printing out the exception name? Or why would it be useful to raise RequestError without the request data that it normally has?

[abhi1693]

Upon reviewing the source code, I did not find any instances where a string is directly passed to the exception. Therefore, it appears that pursuing this change may not be the most effective solution for addressing the issue at hand.

You may have to look into why Celery is re-raising an invalid exception different than what pynetbox raised.

[struppinet]

Yeah I have two 3rd party tools that are not compatible with each other. The issue is open for both. If either of them fixes some code the issue goes away.

Yes you can define you own exception and parameters that's all fine. All I'm asking for is that you be open that the Exception is called as the base class with just a string given which results in the AttributeError. Hence my suggestion to handle it differently if req is just a string, just being backwards compatible.

[2023-04-24 16:11:47,578: CRITICAL/MainProcess] Unrecoverable error: AttributeError("'str' object has no attribute 'status_code'")
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/celery/worker/worker.py", line 203, in start
    self.blueprint.start(self)
  File "/usr/local/lib/python3.10/site-packages/celery/bootsteps.py", line 116, in start
    step.start(parent)
  File "/usr/local/lib/python3.10/site-packages/celery/bootsteps.py", line 365, in start
    return self.obj.start()
  File "/usr/local/lib/python3.10/site-packages/celery/worker/consumer/consumer.py", line 332, in start
    blueprint.start(self)
  File "/usr/local/lib/python3.10/site-packages/celery/bootsteps.py", line 116, in start
    step.start(parent)
  File "/usr/local/lib/python3.10/site-packages/celery/worker/consumer/consumer.py", line 628, in start
    c.loop(*c.loop_args())
  File "/usr/local/lib/python3.10/site-packages/celery/worker/loops.py", line 97, in asynloop
    next(loop)
  File "/usr/local/lib/python3.10/site-packages/kombu/asynchronous/hub.py", line 362, in create_loop
    cb(*cbargs)
  File "/usr/local/lib/python3.10/site-packages/celery/concurrency/asynpool.py", line 325, in on_result_readable
    next(it)
  File "/usr/local/lib/python3.10/site-packages/celery/concurrency/asynpool.py", line 306, in _recv_message
    message = load(bufv)
  File "/usr/local/lib/python3.10/site-packages/pynetbox/core/query.py", line 43, in __init__
    if req.status_code == 404:
AttributeError: 'str' object has no attribute 'status_code'```

[abhi1693]

Since this is not a usecase that pynetbox requires at the moment so a code change is unnecessary to support a 3rd party application.

[abhi1693]

I'm going to close this as there doesn't seem to be any reason to change the exception signature.