search

mathesar-foundation / mathesar

Web application providing an intuitive user experience to databases.
https://mathesar.org/
GNU General Public License v3.0
2.29k stars 316 forks source link

Deleting all columns in a table results in a NoSuchTableError #668

Closed mr-gabe49 closed 11 months ago

mr-gabe49 commented 2 years ago

Description

Expected behavior

To Reproduce

Environment

Full Traceback

Environment:
Request Method: GET
Request URL: http://localhost:8000/mathesar_tables/8/

Django Version: 3.1.12
Python Version: 3.9.7
Installed Applications:
['django.contrib.admin',
 'django.contrib.auth',
 'django.contrib.contenttypes',
 'django.contrib.sessions',
 'django.contrib.messages',
 'django.contrib.staticfiles',
 'rest_framework',
 'django_filters',
 'django_property_filter',
 'mathesar']
Installed Middleware:
['django.middleware.security.SecurityMiddleware',
 'django.contrib.sessions.middleware.SessionMiddleware',
 'django.middleware.common.CommonMiddleware',
 'django.middleware.csrf.CsrfViewMiddleware',
 'django.contrib.auth.middleware.AuthenticationMiddleware',
 'django.contrib.messages.middleware.MessageMiddleware',
 'django.middleware.clickjacking.XFrameOptionsMiddleware']

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/usr/local/lib/python3.9/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/code/mathesar/views.py", line 97, in schema_home
    'common_data': get_common_data(request, database, schema)
  File "/code/mathesar/views.py", line 44, in get_common_data
    'tables': get_table_list(request, schema)
  File "/code/mathesar/views.py", line 35, in get_table_list
    return table_serializer.data
  File "/usr/local/lib/python3.9/site-packages/rest_framework/serializers.py", line 745, in data
    ret = super().data
  File "/usr/local/lib/python3.9/site-packages/rest_framework/serializers.py", line 246, in data
    self._data = self.to_representation(self.instance)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/serializers.py", line 663, in to_representation
    return [
  File "/usr/local/lib/python3.9/site-packages/rest_framework/serializers.py", line 664, in <listcomp>
    self.child.to_representation(item) for item in iterable
  File "/usr/local/lib/python3.9/site-packages/rest_framework/serializers.py", line 502, in to_representation
    attribute = field.get_attribute(instance)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/fields.py", line 457, in get_attribute
    return get_attribute(instance, self.source_attrs)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/fields.py", line 97, in get_attribute
    instance = getattr(instance, attr)
  File "/usr/local/lib/python3.9/site-packages/django/utils/functional.py", line 48, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/code/mathesar/models.py", line 188, in name
    return self._sa_table.name
  File "/usr/local/lib/python3.9/site-packages/django/utils/functional.py", line 48, in __get__
    res = instance.__dict__[self.name] = self.func(instance)
  File "/code/mathesar/models.py", line 168, in _sa_table
    table = reflect_table_from_oid(
  File "/code/db/tables/operations/select.py", line 35, in reflect_table_from_oid
    return reflect_table(table_name, schema, engine, connection_to_use=connection_to_use)
  File "/code/db/tables/operations/select.py", line 12, in reflect_table
    return Table(name, metadata, schema=schema, autoload_with=autoload_with, extend_existing=True)
  File "<string>", line 2, in __new__
    <source code not available>
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/deprecations.py", line 298, in warned
    return fn(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 600, in __new__
    metadata._remove_table(name, schema)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 595, in __new__
    table._init(name, metadata, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 670, in _init
    self._autoload(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 705, in _autoload
    conn_insp.reflect_table(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 788, in reflect_table
    raise exc.NoSuchTableError(table.name)

Exception Type: NoSuchTableError at /mathesar_tables/8/
Exception Value: Fake data
mathemancer commented 2 years ago

It appears SQLAlchemy can't handle reflection of tables which have no columns. I've reproduced this issue on my machine. We'll need to write a custom reflection function if we want to be able to handle this. I think this would be best, since it's a valid state for the DB that we'll want to be able to work with eventually.

bohemia420 commented 2 years ago

@kgodey @mathemancer can I work on this?

mr-gabe49 commented 2 years ago

Hey @bohemia420, @kgodey will be offline for some time, and it's very late in Hong Kong for @mathemancer. I'll assign it to you. Let us know if you have any questions. Thanks!

bohemia420 commented 2 years ago

this is the catch in sqlalchemy reflections.py that's breaking it:

for col_d in self.get_columns(
            table_name, schema, **table.dialect_kwargs
        ):
            found_table = True

            self._reflect_column(
                table,
                col_d,
                include_columns,
                exclude_columns,
                cols_by_orig_name,
            )

firstly, found_table = True if len(self.get_columns) else False would have been a better handling rather than redundantly setting found_table = Truefor say, 1000+ columns in a table.

@mathemancer rather than diverting it to a custom reflections, better to "tweak" the "sqlalchemy" and replace it in our docker image's requirement.txt ?

correcting this puny found_table issue, the UI just loads well that table with albeit 0 columns.

Screenshot 2021-09-26 at 10 33 13 PM
bohemia420 commented 2 years ago

@kgodey In addition to my proposal above being painful syncing/maintaining cherrypicked files, it could 've had other problems:

remediating afresh as I played with SQLAlchemy reflections and what we're calling under the hood, following is the complete troubleshooting, followed by an issue/PR made to SQLAlchemy:

Reproducing and identifying the problem

State with current SQLAlchemy Versions

current version of SQLAlchemy that we use, is 1.4.18

$ docker exec -it mathesar_service_1 sh
# pip3.9 freeze | grep -i sqlalchemy
SQLAlchemy==1.4.18

which, fails to recognise the reflections of empty(no column/keys) tables:

# python3.9
>>> from sqlalchemy import create_engine, Table, Column, Integer, String, MetaData
>>> engine = create_engine('postgresql://mathesar:mathesar@db:5432/mathesar')
>>> engine.table_names()
<stdin>:1: SADeprecationWarning: The Engine.table_names() method is deprecated and will be removed in a future release.  Please refer to Inspector.get_table_names(). (deprecated since: 1.4)
[]
>>> metadata = MetaData(bind=engine)
>>> emp2 = Table('emp2', metadata, )
>>> tab1 = Table('tab1', metadata,Column('id', Integer, primary_key = True), )
>>> metadata.create_all(engine)
>>> engine.table_names()
['emp2', 'tab1']

on checking for/returning those Tables (where exactly NoSuchTableError i.e: return Table(name, metadata, schema=schema, autoload_with=autoload_with, extend_existing=True) inside db/tables/operations/select.py gets raised):

>>> Table('tab1', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Table('tab1', MetaData(bind=Engine(postgresql://mathesar:***@db:5432/mathesar)), Column('id', INTEGER(), table=<tab1>, primary_key=True, nullable=False, server_default=DefaultClause(<sqlalchemy.sql.elements.TextClause object at 0x7ffb2d99cd90>, for_update=False)), schema='public')

A table with atleast one column works, however:

>>> Table('emp2', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 2, in __new__
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/deprecations.py", line 298, in warned
    return fn(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 600, in __new__
    metadata._remove_table(name, schema)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 595, in __new__
    table._init(name, metadata, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 670, in _init
    self._autoload(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 705, in _autoload
    conn_insp.reflect_table(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 788, in reflect_table
    raise exc.NoSuchTableError(table.name)
sqlalchemy.exc.NoSuchTableError: emp2

tables with zero columns i.e no keys e.g emp2, a NoSuchTableError Exception is thrown.

experimenting version upgrade(s)

However, upgrading the version to 1.4.23 proves to be futile, too:

# pip3.9 install SQLAlchemy==1.4.23
# pip3.9 freeze | grep -i sqlalchemy
SQLAlchemy==1.4.23

and executing the same steps as above:

# python3.9
Python 3.9.7 (default, Sep  3 2021, 20:19:49) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from sqlalchemy import create_engine, Table, Column, Integer, String, MetaData
>>> engine = create_engine('postgresql://mathesar:mathesar@db:5432/mathesar')
>>> engine.table_names()
<stdin>:1: SADeprecationWarning: The Engine.table_names() method is deprecated and will be removed in a future release.  Please refer to Inspector.get_table_names(). (deprecated since: 1.4)
[...]
>>> metadata = MetaData(bind=engine)
>>> emp23 = Table('emp23', metadata, )
>>> tab23 = Table('tab23', metadata,Column('id', Integer, primary_key = True), )
>>> metadata.create_all(engine)
>>> engine.table_names()
['emp23', 'tab23', ...]
>>> Table('tab23', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Table('tab23', MetaData(bind=Engine(postgresql://mathesar:***@db:5432/mathesar)), Column('id', INTEGER(), table=<tab23>, primary_key=True, nullable=False, server_default=DefaultClause(<sqlalchemy.sql.elements.TextClause object at 0x7efeaf9ba580>, for_update=False)), schema='public')
>>> Table('emp23', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 2, in __new__
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/deprecations.py", line 298, in warned
    return fn(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 601, in __new__
    metadata._remove_table(name, schema)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 596, in __new__
    table._init(name, metadata, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 671, in _init
    self._autoload(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 706, in _autoload
    conn_insp.reflect_table(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 788, in reflect_table
    raise exc.NoSuchTableError(table.name)
sqlalchemy.exc.NoSuchTableError: emp23
>>>

p.s : Having checked against 1.4.25 also yieleded similar "...NoSuchTableError": emp25

Triaging

solution

no doubt, as stated by @mathemancer, SQLAlchemy can't handle reflections of tables with no columns, and the right fix of that, no matter removing redundant found_table initialisation as we've been digging right, is to rather populate basis presence in self.engine.table_names. With that fix, things begin to work well:

>>> Table('emp_23', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Table('emp_23', MetaData(bind=Engine(postgresql://mathesar:***@0.0.0.0:5432/mathesar)), schema='public')
>>> Table('empidontexist', MetaData(bind=engine), schema='public', autoload_with=engine, extend_existing=True)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<string>", line 2, in __new__
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/deprecations.py", line 298, in warned
    return fn(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 607, in __new__
    metadata._remove_table(name, schema)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py", line 70, in __exit__
    compat.raise_(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/util/compat.py", line 207, in raise_
    raise exception
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 602, in __new__
    table._init(name, metadata, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 677, in _init
    self._autoload(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/schema.py", line 712, in _autoload
    conn_insp.reflect_table(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 774, in reflect_table
    for col_d in self.get_columns(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 497, in get_columns
    col_defs = self.dialect.get_columns(
  File "<string>", line 2, in get_columns
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 55, in cache
    ret = fn(self, con, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/dialects/postgresql/base.py", line 3605, in get_columns
    table_oid = self.get_table_oid(
  File "<string>", line 2, in get_table_oid
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/reflection.py", line 55, in cache
    ret = fn(self, con, *args, **kw)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/dialects/postgresql/base.py", line 3485, in get_table_oid
    raise exc.NoSuchTableError(table_name)
sqlalchemy.exc.NoSuchTableError: empidontexist

steps ahead

I have raised this issue at SQLAlchemy here besides providing a PR to help the issue, here. I'll keep you posted how it is further remediated and merged.

bohemia420 commented 2 years ago

Interestingly, this issue had already been reported to SQLAlchemy, and they've asked us to track the earlier raised issue on this note and closed our issue raised. My PR which they are likely to consider owing to agreeing at fixing found_table in reflection.py for handling "tables with oid and no columns" should fix this issue.

kgodey commented 2 years ago

Thanks for the detailed debug steps @bohemia420! I'll mark this as blocked until the upstream PR is merged.

bohemia420 commented 2 years ago

there's actually a discussion thread on the PR https://github.com/sqlalchemy/sqlalchemy/pull/7118 accompanied by a Gerrit and still under review - somewhere it's being contemplating that the fix in this PR be a part of settings which is bound to raise NoSuchTableError exception by default

kgodey commented 2 years ago

Thanks for the update!

kgodey commented 2 years ago

This is fixed upstream, so we just need to upgrade to SQLAlchemy 1.4.26 or higher. Marking as unblocked.

github-actions[bot] commented 1 year ago

This issue has not been updated in 90 days and is being marked as stale.

dmos62 commented 11 months ago

The referenced PR seems to fix this (judging by the title).