python/cpython

isinstance not work correctly sometimes

Closed this issue · 9 comments

ondratu commented

Bug report

Bug description:

PoorWSGI has OpenAPI Core wrapper: https://github.com/PoorHttp/PoorWSGI/blob/2.6.0/poorwsgi/openapi_wrapper.py#L16

class OpenAPIRequest():
...

which is used in openapi example:
https://github.com/PoorHttp/PoorWSGI/blob/2.6.0/examples/openapi3.py#L80

req.api = OpenAPIRequest(req)
unmarshal_request(req.api, app.openapi_spec)
# in before_response
unmarshal_response(
                req.api or OpenAPIRequest(req),  # when error in before_request
                OpenAPIResponse(res),
                app.openapi_spec)

And openapi_core do check type of request by isinstance:
https://github.com/python-openapi/openapi-core/blob/0.18.2/openapi_core/shortcuts.py#L140

which contains this:

# lines in unmarshal_request
if not isinstance(request, (Request, WebhookRequest)):
        raise TypeError("'request' argument is not type of (Webhook)Request")

# lines in unmarshal_response
if not isinstance(request, (Request, WebhookRequest)):
        raise TypeError("'request' argument is not type of (Webhook)Request")

In openapi example, both of functions are called, first before processing request, second time after processing request. But isinstance in first time return True, which is really not right. But second time, it returns False which is right. I many-times check if really check the right thing, here is logger output which i add to unmarshal_response function:

ERROR:root:instance result: True
ERROR:root:type of request: <class 'poorwsgi.openapi_wrapper.OpenAPIRequest'>
ERROR:root:Request type: <class 'openapi_core.protocols.Request'>
ERROR:root:WebHook type: <class 'openapi_core.protocols.WebhookRequest'>

CPython versions tested on:

3.8, 3.9, 3.10, 3.11, 3.12

Operating systems tested on:

Linux

Eclips4 commented

Hello.
Your code example has two same isinstance cheks, is it intentionally?
Seems you pass there not valid arguments
As I can see, OpenAPIRequest does not inherited from Request or WenhookRequest, so isinstance(OpenAPIRequest(), (Request, WebhookRequest)) returns False as expected.

ondratu commented

Yes, but first time to calling isinstance returns True instead of False. That is the bug.

Please see my log output. And yes double checks of Request is intentional, depends on context.

Eclips4 commented

Yes, but first time to calling isinstance returns True instead of False. That is the bug.

Please see my log output

Can you write a MRE, please?
From the code I see that you inverse the result of isinstance, so False is expected here

ondratu commented

I'm sorry for confusing report. Here is part of code, which i add (the logging) to
https://github.com/python-openapi/openapi-core/blob/0.18.2/openapi_core/shortcuts.py#L140

    if not isinstance(request, (Request, WebhookRequest)):
        raise TypeError("'request' argument is not type of (Webhook)Request")
    import logging
    logging.error("instance result: %s", isinstance(request, (Request, WebhookRequest)))
    logging.error("type of request: %s", type(request))
    logging.error("Request type: %s", Request)
    logging.error("WebHook type: %s", WebhookRequest)

And one more time the output:

ERROR:root:instance result: True
ERROR:root:type of request: <class 'poorwsgi.openapi_wrapper.OpenAPIRequest'>
ERROR:root:Request type: <class 'openapi_core.protocols.Request'>
ERROR:root:WebHook type: <class 'openapi_core.protocols.WebhookRequest'>

I saw some problems with type on Python2.7, which was depends on import, but i never see problems with isinsance. So it should be hard to create MRE.

Eclips4 commented

Can you show the mro of request?
type(request).__mro__

ondratu commented 
logging.error("mro of request: %s", type(request).__mro__)
ERROR:root:mro of request: (<class 'poorwsgi.openapi_wrapper.OpenAPIRequest'>, <class 'object'>)
Eclips4 commented 
logging.error("mro of request: %s", type(request).__mro__)
ERROR:root:mro of request: (<class 'poorwsgi.openapi_wrapper.OpenAPIRequest'>, <class 'object'>)

Request type is marked as @runtime_checkable: https://github.com/python-openapi/openapi-core/blob/0.18.2/openapi_core/protocols.py#L28

So, there's no error. Here you can read what is @runtime_checkable: https://docs.python.org/3.12/library/typing.html#typing.runtime_checkable

ondratu commented

Ohh, thanks for your time.

The problem is that in first function unmarshal_request object is instance of Response, but the same test in unmarshal_response isn't instance. Dumps looks same, i try to find out, what's happen. Somewhere there is still bug, but apparently not in isinstance, cause inheriting OpenAPIRequest(Request) helps.

ondratu commented

Ok, i got it. And hard to say if is bug, but for my opinion, there are few bugs hidden after bad user (programmer) implementation.

Here is MRE:

import typing
import logging


@typing.runtime_checkable
class Boo(typing.Protocol):

    @property
    def boo(self) -> str:
        ...

    @property
    def data(self) -> str:
        ...


@typing.runtime_checkable
class Foo(Boo, typing.Protocol):

    @property
    def foo(self) -> str:
        ...


class MyClass():

    @property
    def foo(self):
        logging.error("error in foo")
        raise RuntimeError("No foo")

    @property
    def boo(self):
        logging.error("error in boo")
        raise RuntimeError("No boo")

    @property
    def data(self):
        logging.error("error data")
        self.nothing
        return "data"


myc = MyClass()
print(isinstance(myc, Boo))

Environment of bug:

  • The Protocol class which is tested in isinstance must to be inherited of another Protocol class
  • It must be error in access to property -> i mean reading the property (which is really error of user, not in isinstance)

What happen:

  • When inherited Protocol class is checking, it probably checks it's properties (values) - that should be documented! And it happen only for attributes in base Protocol class - may be this should be bug too.
  • The properties are checking in random order -> that is fine for me, but it goes to random (undefined) behavior - see output, and random behavior is bug too, but when it will be fix next bug, this is ok.
  • At the end, if there is exception in reading property like in boo method, i see it, but if i access to not existing attribute like in data, i don't. And that is bug for my opinion.

I got these too outputs:

ERROR:root:error in boo
Traceback (most recent call last):
  File "/home/mcbig/mre_isinstance.py", line 45, in <module>
    print(isinstance(myc, Boo))
          ^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/typing.py", line 2006, in __instancecheck__
    if all(hasattr(instance, attr) and
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.11/typing.py", line 2006, in <genexpr>
    if all(hasattr(instance, attr) and
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/mcbig/mre_isinstance.py", line 36, in boo
    raise RuntimeError("No boo")
RuntimeError: No boo

or

ERROR:root:error data
False