superset.views.datasource.views.Datasource does not implement a response_400

[cancan101]

When this error handler is hit:
https://github.com/dpgaspar/Flask-AppBuilder/blob/2d505e819d496308a34c5f623d70331a37599cc4/flask_appbuilder/api/__init__.py#L145-L146 in the superset.views.datasource.views.Datasource view, a a further Exception is raised as there is no 400 handler for that view:

Traceback (most recent call last):
  File "/Users/alex/.pyenv/versions/3.7.3/envs/app/lib/python3.7/site-packages/superset/views/base.py", line 203, in wraps
    return f(self, *args, **kwargs)
  File "/Users/alex/.pyenv/versions/3.7.3/envs/app/lib/python3.7/site-packages/flask_appbuilder/api/__init__.py", line 145, in wraps
    return self.response_400(
AttributeError: 'Datasource' object has no attribute 'response_400'

How to reproduce the bug

1. Use legacy datasource editor
2. Define a table with an = in its name
3. Click sync columns from source

Expected results

Ideally it should handle the table name (it looks like the string is not properly rison encoded. If it isn't going to allow these strings, then at the very least the 400 error should be properly generated

Actual results

A toast showing: "'Datasource' object has no attribute 'response_400'"

Screenshots

The dataset:

The error toast:

Environment

(please complete the following information):

• browser type and version: Chrome 98.0.4758.102
• superset version: Superset 1.4.1
• python version: Python 3.7.3
• node.js version: v15.4.0
• any feature flags active: n/a

Checklist

Make sure to follow these steps before submitting your issue - thank you!

• I have checked the superset logs for python stacktraces and included it here as text if there are any.
• I have reproduced the issue with at least the latest released version of superset.
• I have checked the issue tracker for the same issue and I haven't found one similar.

Additional context

Add any other context about the problem here.

[zhaoyongjie]

Hi @cancan101, I tested this issue on the latest master. The database table containing = can be added in Superset either legacy FAB UI or newest UI. Could you retry in the latest master branch?

Legacy FAB UI

Newest UI

[cancan101]

So it looks like #18056 did fix the rison encoding meaning that the 400 error is no longer generated by that code path. That being said, if something else does generate a 400 error at some point the view does not correctly handle it and will then raise an error that response_400 is not defined. This can be tests using a CLI tool and GETing against /datasource/external_metadata_by_name/ with q=<invalid rison string>.

[zhaoyongjie]

Hi @cancan101, thanks for the debug. Today, I tested the endpoint in my local test environment. It looks works.

[cancan101]

I just ran this:

curl 'http://127.0.0.1:8088/datasource/external_metadata_by_name/?q=(database_name:SWAPI,datasource_type:table,schema_name:main,table_name:allSpecies?foo=1)' ....

(omitting the full curl line)
and got back:

{"error": "'Datasource' object has no attribute 'response_400'"

and this is a 500 error ^

this is running against master.

[zhaoyongjie]

@cancan101 You should quote the table_name with single quotation.

curl "http://127.0.0.1:8088/datasource/external_metadata_by_name/?q=(database_name:SWAPI,datasource_type:table,schema_name:main,table_name:'allSpecies?foo=1')" ....

[cancan101]

Right , I intentionally misquoted it to be consistent with the prior broken rison escaping logic. That logic is now fixed in Master however this endpoint should return a 400 error not a 500 error in the case of a malformed query param.

[zhaoyongjie]

Hi @cancan101, Thanks for pointing this out.

The endpoint external_metadata_by_name base on a BaseSupersetView. BaseSupersetView hasn't response_400 method, so that when we invoke self.response_400(message="Not a valid rison argument") in rison decorator, it raise a 500 exception.

I think we have 2 ways to fix this.

1. Use BaseSupersetModelRestApi instead of BaseSupersetView in datasource.views
2. The rison decorator raise a ValueError exception instead a HTTP response. IMO, the rison decorator should decouple with HTTP request handler, we should process HTTP exception in the HTTP handler.

@dpgaspar What do you think about this issue?

[eschutho]

@zhaoyongjie We're doing some work on the dataset models and apis. I think we can take this on and migrate the datasource api to v1, which would use the restApi base model.

[cici1210]

Check if your dataset name contains special characters, like "&".

[rusackas]

Also tagging @hughhhh here since I know he's working on some API V1 refactors/migration currently.

[rusackas]

Closing this as stale since it's been silent for so long, and we're trying to steer toward a more actionable Issues backlog. If people are still encountering this in current versions (currently 3.x) please re-open this issue, open a new Issue with updated context, or raise a PR to address the problem. Thanks!