SceneInventory: Ignore containers with invalid UUID in representation

iLLiCiTiT commented 3 months ago

Changelog Description

Fake representation id of containers with invalid representation id.

Additional info

This is proposal of possible issue with using scenes created in OpenPype. Proposed solution does show the container in UI but as invalid so user can see it. API functions are silently ignoring containers with invalid representation id.

Another solution would be to completelly ignore it even in UI to avoid other possible issues.

Testing notes:

Have scene with referenced content from OpenPype.
Open scene inventory.
It should not crash.
ValidateContainers should not crash either.

Resolves https://github.com/ynput/ayon-core/issues/703

BigRoy commented 3 months ago

The code stops here without raising/printing/logging any error or warning: https://github.com/ynput/ayon-core/blob/403442b281b0645952fc3b9f6513e8c8c27ba88c/client/ayon_core/tools/sceneinventory/model.py#L151-L153

BigRoy commented 3 months ago

And here are the details when logging the exception explicitly:

Traceback (most recent call last):
  File "E:\dev\ayon-core\client\ayon_core\tools\sceneinventory\model.py", line 153, in refresh
    version_items_by_product_id = self._controller.get_version_items(
  File "E:\dev\ayon-core\client\ayon_core\tools\sceneinventory\control.py", line 114, in get_version_items
    return self._containers_model.get_version_items(product_ids)
  File "E:\dev\ayon-core\client\ayon_core\tools\sceneinventory\models\containers.py", line 312, in get_version_items
    version_entities = list(ayon_api.get_versions(
  File "C:\Program Files\Ynput\AYON 1.0.2\dependencies\ayon_api\server_api.py", line 4564, in get_versions
    for parsed_data in query.continuous_query(self):
  File "C:\Program Files\Ynput\AYON 1.0.2\dependencies\ayon_api\graphql.py", line 380, in continuous_query
    raise GraphQlQueryFailed(
ayon_api.exceptions.GraphQlQueryFailed: GraphQl query Failed: Variable '$productIds' got invalid value None at 'productIds[2]'; Expected non-nullable type 'String!' not to be None. (Line 1 Column 43)A

It seems that any error in the refresh method doesn't raise any actual error or log it anywhere. This may be due to how the Qt UI processes it (it's maybe in a callback or whatever?)

As soon as you discard that None then you get to another error. It seems that none of the involved logic considers that there may be containers that it can't resolve into a valid container.

BigRoy commented 3 months ago

Still an empty scene inventory and no errors visible in console.

With some changes the stack trace now is:

Traceback (most recent call last):
  File "E:\dev\ayon-core\client\ayon_core\tools\sceneinventory\model.py", line 129, in refresh
    self.__refresh(selected)
  File "E:\dev\ayon-core\client\ayon_core\tools\sceneinventory\model.py", line 240, in __refresh
    repre_info.representation_name
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

Note that the line numbers might be off because I had to put the whole refresh in a try except to get any error. I basically added the lines:

    def refresh(self, selected=None):
        try:
            self.__refresh(selected)
        except Exception:
            self.log.error("Error occurred during refresh.", exc_info=True)

    def __refresh(self, selected=None):
        # basically the old refresh method below..
        """Refresh the model"""
        # for debugging or testing, injecting items from outside

Somehow due to how refresh gets called via Qt it seems to report no error or stack trace?

Anyway, it basically fails on:

                unique_name = (
                    repre_info.representation_name
                    + container_item.object_name or "<none>"
                )

When changing that to:

                unique_name = (
                    repre_info.representation_name or "<unknown_representation>"
                    + container_item.object_name or "<none>"
                )

I get it to show!

~~Maybe the N/A entities should be grouped together?~~ Nevermind, it's grouping per invalid representation id | that's correct behavior I just broke the values to two different broken representation ids in this example scene. ✅

What's nice though is that removing of items does work!

So small fixes left - and then this is great.

It might be nice to put in that "try except" hack or maybe an extra log_errors decorator on the method to force that for us so that if an error occurs in the future we actually get some logs for it.

iLLiCiTiT commented 3 months ago

Not something we need to fix here - just seemed somewhat related and I wanted to mention it somewhere

It is not handled on multiple places, definetelly should be. But indeed different PR.

ynput / ayon-core