`Except: pass` in databases metastore

[mrvisscher]

There is an undefined Except: pass in the database deletion procedure. This causes databases to be silently deleted from the metastore regardless of any errors that occurred during actual database deletion from the SQLite database. This leads to ghost databases that are not registered but do still exist.

https://github.com/brightway-lca/brightway2-data/blob/main/bw2data/meta.py#L100

def __delitem__(self, name):
    from . import Database
    try:
        Database(name).delete(warn=False)
    except:
        pass

    super(Databases, self).__delitem__(name)

[tngTUDOR]

Do you suggest we try:

def __delitem__(self, name):
    from . import Database
    try:
        Database(name).delete(warn=False)
        super(Databases, self).__delitem__(name)
    except e:
        logging.error("Something went wrong removing the db", e)

to start with, and review the whole "try/except/finally" of this feat+method ?

[cmutel]

@tngTUDOR No, I think he is saying the except clause should restore the database to databases.

@mrvisscher Can you give me an example of what kind of error could arise during the SQL transaction? It should be pretty straightforward and I wouldn't have expected any errors there.

BTW, this is another thing that should change ASAP - the database and method data (even CFs) will be in the database as well.

[tngTUDOR]

I know this is probably unrelated but some tests in 3.11 @ win arch fail with some "delete" related ops (for projects, not databases):

https://github.com/brightway-lca/brightway2-data/actions/runs/10860661234/job/30141531219#step:6:464

just leaving it here in case this could help out the coverage and / or identification of the reason why we have such test failing

[mrvisscher]

Do you suggest we try:

def __delitem__(self, name):
    from . import Database
    try:
        Database(name).delete(warn=False)
        super(Databases, self).__delitem__(name)
    except e:
        logging.error("Something went wrong removing the db", e)

to start with, and review the whole "try/except/finally" of this feat+method ?

Yes this was actually what I was suggesting. We had the Database Delete action in the Activity Browser failing silently, and our tests didn't catch it because the exception never came up.

@mrvisscher Can you give me an example of what kind of error could arise during the SQL transaction? It should be pretty straightforward and I wouldn't have expected any errors there.

Any error from database lock to whatever, depending on the backend.

BTW, this is another thing that should change ASAP - the database and method data (even CFs) will be in the database as well.

Well that's just fantastic. I was discussing this at our side not so long ago as well. Happy to hear that's being planned.