[table] feat: REST API by dpgaspar · Pull Request #9034 · apache/superset

dpgaspar · 2020-01-28T16:29:41Z

SUMMARY

Following CRUD MVC deprecation. CRUD REST API for tables

ADDITIONAL INFORMATION

REVIEWERS

craig-rueda · 2020-01-28T17:39:55Z

+logger = logging.getLogger(__name__)
+
+
+def validate_database(value):


Nit: value->db_id . The name value is a bit ambiguous

Was just following def __call__(self, value) from marshmallow, but yes, I'll change that

craig-rueda · 2020-01-28T17:53:55Z

+
+    @validates_schema
+    def validate_schema(self, data: Dict):  # pylint: disable=no-self-use
+        validate_table_uniqueness(data)


Mixing API schema validation and business logic/db validation feels like a bad pattern here. Marshmallow should be concerned with keeping requests in/out of the API consistent, whereas data access rules and/or auth should live inside the ViewController.

One other interesting side note - these checks could potentially trigger BEFORE auth validation happens, which could lead to leakage (a user without rights to a DB for instance could phish around for DB names which would trigger these replies if auth logic happened downstream of this)

Agreed, good catch.

The schema validation is debatable, since we are only performing basic data validation (does the relation exist?, is the fields unique?). Yet I have mixed feelings about it. The other approach would use pre_, post_ hooks. Note that using marshmallow validation gives us proper error messages for free.

It's not possible for the validation to trigger before any auth validation. Since this is done at the view decorator level. Yet, it's certainly a good point since it's possible further down the road for mashmallow validation to perform some kind of data authorization.

I would say this is a key issue. Marshmallow data validation is really useful, but has some constrains, and may easily force unnecessary db round trips (notice the use of the flask g on this case, that I'm not a big fan). We can rollback this pattern (it already implemented on previous new API PR's) and rollback to using pre_, post_ hooks that can call specific business logic API outside of the view scope.

I don't think it's one or the other. I think marshmallow validation is great for validating the shape of the request. I do not think that marshmallow should be running database queries. My recommendation would be for the validation/auth logic to look something like:

Can the user access the route? If yes, continue, if no 403

Run marshmallow validation to validate the shape of the request. If OK, continue, if not 400

Look up the resource being modified. Can the user modify it? If so, continue, if not 403

Attempt the resource modification.

I think that can be accomplished via some combination of marshmallow and pre_ and post_ hooks, but you're the expert :)

This can also become a security issue quite easily. If we are doing data validation inside of marshmallow schemas, an attacker could (potentially) use validation errors to gather information about a resource that they should not have access to. The schema must be very carefully written and thoroughly tested to prevent this. Much better to separate schema validation/resource checks as described by @willbarrett

codecov-io · 2020-02-10T15:17:10Z

Codecov Report

Merging #9034 into master will decrease coverage by 0.33%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   #9034      +/-   ##
=========================================
- Coverage   59.43%   59.1%   -0.34%     
=========================================
  Files         369     372       +3     
  Lines       11743   11920     +177     
  Branches     2884    2917      +33     
=========================================
+ Hits         6980    7045      +65     
- Misses       4584    4693     +109     
- Partials      179     182       +3

Impacted Files	Coverage Δ
superset/assets/src/components/Checkbox.jsx
.../assets/src/dashboard/reducers/dashboardFilters.js
...ets/src/dashboard/components/dnd/DragDroppable.jsx
superset/assets/src/components/EditableTitle.jsx
...src/explore/components/controls/VizTypeControl.jsx
...t/assets/src/components/InfoTooltipWithTrigger.jsx
.../src/dashboard/components/UndoRedoKeylisteners.jsx
.../src/dashboard/components/FilterTooltipWrapper.jsx
...sets/src/dashboard/components/DashboardBuilder.jsx
...ssets/src/dashboard/components/PublishedStatus.jsx
... and 709 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1867f06...e737516. Read the comment docs.

dandanhub · 2020-02-11T19:02:07Z

I am also trying to add REST API for table 9118. It is good to know we've already been working on the same feature. Do we have a plan on when this PR will be merged into master?

villebro

Some minor non-blocking comments.

villebro · 2020-02-12T21:19:15Z

 decorator==4.4.1          # via retry
 defusedxml==0.6.0         # via python3-openid
-flask-appbuilder==2.2.2
+flask-appbuilder==2.2.3rc5


I think this needs a rebase

villebro · 2020-02-12T21:41:50Z

+    include_route_methods = RouteMethod.REST_MODEL_VIEW_CRUD_SET | {RouteMethod.RELATED}
+    openapi_spec_tag = "Datasets"
+
+    list_columns = ["metric_name", "verbose_name", "metric_type"]


Might be overkill (some metrics might be very long) and probably depends on where this is used in practice, but for a listing I think I would prefer to see the expression over metric_type here. Looking at the datasource editor, it seems it's also doing the same:

villebro · 2020-02-12T21:44:57Z

+        "type": "VARCHAR",
+        "groupby": False,
+        "filterable": True,
+        "expression": "col expression",


nit: perhaps put some ANSI SQL here: case when 1 = 1 then 'abc' else 'qwerty' end

villebro · 2020-02-12T21:49:47Z

+        verbose_name: str = "",
+        description: str = "",
+        type: str = "",
+        groupby: bool = True,
+        filterable: bool = True,
+        is_dttm: bool = True,
+        expression: str = "",
+        python_date_format: str = "",


Is there some reason the strs aren't Optional[str] = None? In the datamodel these seem to be nullable.

dpgaspar · 2020-03-11T10:24:24Z

Replaced by #9268

[table] feat: REST API

745bf72

superset-github-bot Bot added the preset-io label Jan 28, 2020

pull-request-size Bot added the size/XL label Jan 28, 2020

[table] fix: list

1867f06

dpgaspar marked this pull request as ready for review January 28, 2020 16:58

dpgaspar requested review from craig-rueda and villebro January 28, 2020 17:01

craig-rueda requested changes Jan 28, 2020

View reviewed changes

dpgaspar added 6 commits February 6, 2020 12:25

Merge remote-tracking branch 'upstream/master' into feature/api-tables

9cf0300

[table] Add column CRUD REST API on table namespace

1aa5d21

[tables] New CRUD for columns

f6b0636

[table] Tests for add columns and table col security filters

f4141f8

[table] more tests

8cc9084

[table] fix lint

e737516

[table] CRUD for table metrics

23d3074

dpgaspar mentioned this pull request Feb 11, 2020

Add TableRestApi #9118

Closed

12 tasks

villebro approved these changes Feb 12, 2020

View reviewed changes

dpgaspar closed this Mar 11, 2020

willbarrett deleted the feature/api-tables branch March 11, 2020 17:34

		logger = logging.getLogger(__name__)


		def validate_database(value):

Conversation

dpgaspar commented Jan 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CATEGORY

SUMMARY

ADDITIONAL INFORMATION

REVIEWERS

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgaspar Jan 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

suddjian Feb 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Feb 10, 2020

Codecov Report

Uh oh!

dandanhub commented Feb 11, 2020

Uh oh!

villebro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgaspar commented Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dpgaspar commented Jan 28, 2020 •

edited

Loading

dpgaspar Jan 30, 2020 •

edited

Loading

suddjian Feb 4, 2020 •

edited

Loading