InvalidDatetimeFormat error when attempting to filter on a partially-entered date

[seancolsen]

Steps to reproduce

1. Open the table page for table with a Date column, e.g. the "all_data_types" data set.

2. Add a filter condition specifying the date column to be equal to a date. Then begin entering a date starting with one number.

   

3. Expect either to see no results or to see the same results as without the filter condition.

4. Instead, observe that the GET request to the records endpoint responds with an error 500 and the following traceback

Traceback

Environment:


Request Method: GET
Request URL: http://localhost:8000/api/db/v0/tables/709/records/?limit=500&offset=0&filter=%7B%22equal%22%3A%5B%7B%22column_id%22%3A%5B2917%5D%7D%2C%7B%22literal%22%3A%5B%227%22%5D%7D%5D%7D

Django Version: 3.1.14
Python Version: 3.9.9
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',
 'mathesar.middleware.CursorClosedHandlerMiddleware']



Traceback (most recent call last):
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1770, in _execute_context
    self.dialect.do_execute(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 717, in do_execute
    cursor.execute(statement, parameters)

The above exception (invalid input syntax for type date: "7"
LINE 4: WHERE date = '7'), 
                     ^
) was the direct cause of the following exception:
  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 "/usr/local/lib/python3.9/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/viewsets.py", line 125, in view
    return self.dispatch(request, *args, **kwargs)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 466, in handle_exception
    response = exception_handler(exc, context)
  File "/code/mathesar/exception_handlers.py", line 55, in mathesar_exception_handler
    raise exc
  File "/usr/local/lib/python3.9/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/code/mathesar/api/db/viewsets/records.py", line 68, in list
    records = paginator.paginate_queryset(
  File "/code/mathesar/api/pagination.py", line 75, in paginate_queryset
    self.count = table.sa_num_records(filter=filters, search=search)
  File "/code/mathesar/models/base.py", line 454, in sa_num_records
    return get_count(
  File "/code/db/records/operations/select.py", line 98, in get_count
    return execute_pg_query(engine, relation)[0][col_name]
  File "/code/db/utils.py", line 32, in execute_pg_query
    return execute_statement(engine, executable, connection_to_use=connection_to_use).fetchall()
  File "/code/db/utils.py", line 18, in execute_statement
    return conn.execute(statement)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/future/engine.py", line 280, in execute
    return self._execute_20(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1582, in _execute_20
    return meth(self, args_10style, kwargs_10style, execution_options)
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/sql/elements.py", line 324, in _execute_on_connection
    return connection._execute_clauseelement(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1451, in _execute_clauseelement
    ret = self._execute_context(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1813, in _execute_context
    self._handle_dbapi_exception(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/base.py", line 1994, in _handle_dbapi_exception
    util.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/engine/base.py", line 1770, in _execute_context
    self.dialect.do_execute(
  File "/usr/local/lib/python3.9/site-packages/sqlalchemy/engine/default.py", line 717, in do_execute
    cursor.execute(statement, parameters)

Exception Type: DataError at /api/db/v0/tables/709/records/
Exception Value: (psycopg2.errors.InvalidDatetimeFormat) invalid input syntax for type date: "7"
LINE 4: WHERE date = '7'), 
                     ^

[SQL: WITH anon_2 AS 
(SELECT public.all_types.id AS id, public.all_types.text AS text, public.all_types.number AS number, public.all_types.money AS money, public.all_types.boolean AS boolean, public.all_types.date AS date, public.all_types.date_time AS date_time, public.all_types.time AS time, public.all_types.duration AS duration, public.all_types.email AS email, public.all_types.uri AS uri 
FROM public.all_types 
WHERE date = %(param_1)s), 
anon_1 AS 
(SELECT count(%(count_1)s) AS _count 
FROM anon_2)
 SELECT anon_1._count 
FROM anon_1]
[parameters: {'count_1': 1, 'param_1': '7'}]
(Background on this error at: http://sqlalche.me/e/14/9h9h)

Notes

• This also affects filtering via the search_fuzzy parameter used by the Record Selector.

Implementation

• In theory, this could be fixed on the front end by, but I think a back end fix would make more sense.
• As for the expected behavior here, I'm leaning towards ignoring invalid dates as filter conditions.

CC @pavish @dmos62 @mathemancer

[dmos62]

Thanks for report.

My immediate reaction is that this makes sense, since 7 can't be sensibly parsed into a date. It's just that it should be a 4xx, not a 500.

Is frontend currently able to handle this gracefully?

I'm leaning towards ignoring invalid dates as filter conditions

We don't actually do any parsing of dates ourselves, we just forward them to Postgres. We're not setup to ignore a filter if it causes Postgres to throw.

[seancolsen]

We're not setup to ignore a filter if it causes Postgres to throw.

Ok in that case I think we should handle the bulk of the fix here in the front end. As such I'm re-labeling this ticket as front end. Modifying the error response on the back end would be nice, but I'd consider that change to be low-priority enough that we can disregard it for now.

[seancolsen]

Without having yet looked at any code, my inclination would be to fix this by making the DateInput component (and similar components) bind its value to null or undefined (whichever is most appropriate, given the existing design of the component) when the user-entered value appears to be incomplete. This is somewhat tricky though. User-entered values like today cannot be parsed by the front end into canonical dates, but they still need to be passed to the back end because they can be parsed by Postgres.

@pavish can you see an easy path forwards to fixing this on the front end? Would you be up for taking this ticket since it's code you've worked on?

[dmos62]

@seancolsen I was imagining that frontend would throw its queries at our run endpoint, without parsing that date input, and only keep our response if it's a 200. Minimizing how often you hit the run endpoint is nice, but not strictly necessary, I think.

[pavish]

We decided (after several to-and-fro conversations) that we will not be performing any sort of date validation on the frontend, since we'd want to be able to accept any string that the backend (right now it's postgres) can parse. The DateInput component is built around this. Restricting values to only what the frontend can parse will severely limit the functionality of the component.

when the user-entered value appears to be incomplete

We do not know whether the value is incomplete or not, because the component was built to accept any string.

can you see an easy path forwards to fixing this on the front end?

I don't think we can fix this on the frontend with the above case in mind. I'd very much like the solution to come from the backend for handling invalid filters.

[seancolsen]

@dmos62

I was imagining that frontend would throw its queries at our run endpoint, without parsing that date input, and only keep our response if it's a 200

I'm confused by this message. The run endpoint is for the Data Explorer, right? This ticket is about the records endpoint used by the Table Page and the Record Selector. I don't see how we could use the run endpoint here.

It's also worth mentioning that when reproducing this issue, the records endpoint takes about 13 seconds to issue that 500 response. That slowness makes it difficult for the front end to make decisions about whether or not to "keep" the response as you're suggesting. If we take an approach where the front end tries to request records and only keeps the response in some cases, we'll need to improve the response time here and we'll need a more specific (and conformant) error response that will allow the front end to differentiate this error response from other error responses.

[dmos62]

the records endpoint takes about 13 seconds to issue that 500 response

😲 I thought we had minimized our response times to milliseconds for all frequently used endpoints; I'm confused. You mean when a user loads the table page, he's having to wait at least 13 seconds to see the records?

I was imagining that frontend would throw its queries at our run endpoint, without parsing that date input, and only keep our response if it's a 200

I'm confused by this message. The run endpoint is for the Data Explorer, right? This ticket is about the records endpoint used by the Table Page and the Record Selector. I don't see how we could use the run endpoint here.

I was under the false impression that we have replaced table records endpoint with query run endpoint, but now that I think about it it's the query records endpoint that's not used anymore.

Either way, I still picture the workflow here being that frontend throws whatever it has at the backend, the backend throws it at Postgres, and we see what sticks. We can't (and don't want to) parse these inputs neither on the frontend nor on the backend.

[seancolsen]

@dmos62 no, the records endpoint doesn't always take 13 seconds -- it just takes 13 seconds when reproducing this issue. So if we're going to "throw it at Postgres and see what sticks", we'll need to find a way to do that in a more performant manner.

[dmos62]

I'd say that the performance of records endpoint and how to deal with invalid filter specs are two different problems. Former might be latter's prerequisite though.

[pavish]

I'm not sure why this issue is categorized as 'frontend'. I believe we need further discussion before we have an action item to perform as part of this issue. I'm marking this as draft.

[kgodey]

Assigned to @seancolsen to drive this forward to a conclusion.

[dmos62]

There was an extended discussion about this on Matrix (which I missed), and we scheduled a call to continue it, so in preparation, and for the record, I'll post my opinion here:

Not sure why we're so hell bent on validating elements of user input in isolation (i.e. finding exactly which part of a query's definition is broken). Any way to do that seems inefficient or complicated.

I think the solution is in UX. Have user's input only be persisted when we can successfully run the query his input defines. If backend returns something other than 200, you know that either your input is partial or invalid. Neither the frontend, nor the backend doesn't know which part of the query definition is to blame, but the user knows, because he knows what changes he's making that aren't being persisted. The fact that these changes aren't being persisted means that we don't have to care that we don't know which part of the query is the problem.

[seancolsen]

I'm closing this ticket because I've opened #2327 with a more specific plan that we formulated during a meeting. I've put it into the "Backlog" milestone since it will require some more work than I think we have time to handle before our release.