SuiteFactory.delete(...) raises a TypeException when using a DatabaseStoreBackend

[smcl]

Describe the bug
When you configure an ExpectationStore with a DatabaseStoreBackend and try to delete a suite, a TypeException is raised.

To Reproduce

I've got a very small project which demonstrates this in action at https://github.com/smcl/gx-dbstore-delete-issue - you just need to ensure you have great-expectations 1.5.x and sqlalchemy installed.

But basically the code looks like the below

import great_expectations as gx
context = gx.get_context(mode="file")
context.suites.add(gx.core.expectation_suite.ExpectationSuite(name="test_suite"))
context.suites.delete("test_suite")

With a very "vanilla" great_expectations.yml with an expectations_store configured as follows:

  expectations_store:
    # class_name: ExpectationsStore
    # store_backend:
    #   class_name: TupleFilesystemStoreBackend
    #   base_directory: expectations/
    class_name: ExpectationsStore
    store_backend:
      class_name: DatabaseStoreBackend
      credentials:
        drivername: sqlite
        database: ./gx.db

When you run the script you'll encounter the following Exception:

    for key_col, val in zip(self.key_columns, key)
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: 'ExpectationSuiteIdentifier' object is not iterable

Expected behavior
The provided app.py script should terminate without any errors.

Environment (please complete the following information):

• Operating System: Windows
• Great Expectations Version: 1.5.6
• Data Source: n/a
• Cloud environment: n/a

Additional context

I looked into the GX sources to see if I was doing something stupid. The TypeError is raised inside DatabaseStoreBackend.remove_key which reports that "'ExpectationSuiteIdentifier' object is not iterable" (code is here) - object in question is the key parameter below which we pass into zip(...):

    def remove_key(self, key):  # type: ignore[explicit-override] # FIXME
        delete_statement = self._table.delete().where(
            sa.and_(
                *(
                    getattr(self._table.columns, key_col) == val
                    for key_col, val in zip(self.key_columns, key)
                )
            )
        )
        # ... removed

There is no type annotation for key here, but this code appears to believe that key is a collection that can be iterated over ... but in this case it is called indirectly from SuiteFactory.delete here:

    key = self._store.get_key(name=suite.name, id=suite.id)
    self._store.remove_key(key=key)

That get_key method has a type annotation indicating that it returns either GXCloudIdentifier | ExpectationSuiteIdentifier - neither of these types are iterable so it will obviously cause an exception when it is then passed as an argument into zip().

[smcl]

I wrote a custom hacked DatabaseStoreBackend with a dodgy workaround just to experiment a bit. And doing a bit more debugging of something else I think I figured out what's going on here.

So when we simply call the SuiteFactory.get(self, name: str) method to retrieve a suite by name one thing it does is check whether that name exists in the store in question. It does this using the base Store class's has_key(self, key: DataContextKey) method which looks like this (I've included the remove_key method too for comparison as it immediately follows):

    def has_key(self, key: DataContextKey) -> bool:
        if key == StoreBackend.STORE_BACKEND_ID_KEY:
            return self._store_backend.has_key(key)
        else:
            if self._use_fixed_length_key:
                return self._store_backend.has_key(key.to_fixed_length_tuple())
            return self._store_backend.has_key(key.to_tuple())

    def remove_key(self, key):
        return self.store_backend.remove_key(key)

The DatabaseStoreBackend._has_key method has a similarly constructed query based on that zip(self.key_columns, key) expression. But in this case it makes sense since we've passed in a tuple[str] rather than a str.

So I think what has happened is that remove_key was supposed to also have similar code to has_key which builds the key into a tuple before passing it to the appropriate store backend method. The other backends work around this by building a tuple (InMemoryStoreBackend, TupleFileSystemStoreBackend, GXCloudStoreBackend).

Sorry if these extra notes are unhelpful (or wrong!) I was just doing a little braindump in case it helps in any way.