Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SIP-17] Proposal for using new FAB REST API #7259

Closed
dpgaspar opened this issue Apr 10, 2019 · 5 comments
Closed

[SIP-17] Proposal for using new FAB REST API #7259

dpgaspar opened this issue Apr 10, 2019 · 5 comments
Labels
sip Superset Improvement Proposal

Comments

@dpgaspar
Copy link
Member

dpgaspar commented Apr 10, 2019

Motivation

Take advantage of the new FAB REST API feature available since version 1.13.0. The previous
API endpoints available inside ModelView did not follow a strict RESTful convention, and will be removed in 2 to 3 minors. The following are some improvements:

  • API resource protection using JWT and/or flask-login signed cookies (current authentication method). API defaults to JWT only.
  • Optional CRUD RESTful API using similar class overrides as ModelViews
  • Delegate base API support like handling exceptions to FAB.
  • Leverage Rison style URI arguments out of the box with optional JSON schema validation
  • OpenAPI automatic spec generation for CRUD, and easy generation for BaseApi method endpoints

Docs: https://flask-appbuilder.readthedocs.io/en/latest/rest_api.html

Proposed Change

I propose migrating current views.Api class to BaseApi. So Superset will have it's new API properly anchored on /api/v1/<resource>/ and start using FAB's new feature.

  • The endpoint /api/v1/query does not need to change it's route or HTTP method.

  • The endpoint /api/v1/form_data/ would be changed to /api/v1/slice/ using ModelRestApi on readonly mode (can_get, can_info only permissions).

New or Changed Public Interfaces

This SIP affects the following API endpoints:

  • /api/v1/form_data GET -> /api/v1/slice GET: exposing a get item and get list leveraging complex filters, pagination, internationalization, ordering, custom select columns (all the way to the database)

Optional Swagger UI for OpenAPI spec visualization.

New dependencies

New dependencies were added to FAB 1.13.0:

  • marshmallow
  • marshmallow_enum
  • marshmallow-sqlalchemy
  • Flask-JWT-Extended
  • prison
  • apispec[yaml]

Migration Plan and Compatibility

A side by side migration plan can be achieved. Since the new API endpoint is exposed on a different route. Access to the old endpoint would log a deprecation log message.

Rejected Alternatives

@kristw kristw added the sip Superset Improvement Proposal label Apr 10, 2019
@DiggidyDave
Copy link
Contributor

This is great. I think the deprecation migration plan is a requirement. (wasn't sure about the language "can be achieved" so just making sure)

@dpgaspar
Copy link
Member Author

I've submitted #7482, but needs FAB>2.1. This PR has no breaking changes.

For the deprecation of the old REST API attached on the ModelView's, I'll be submitting code to replace them. Currently Superset's setup.py is pinned under the deprecation version.
But I would say that this is a different problem outside the context of this SIP.

@rusackas
Copy link
Member

@dpgaspar Should this SIP go up for a VOTE?

@villebro
Copy link
Member

I believe this can be closed as it's effectively implemented.

@dpgaspar
Copy link
Member Author

Yes, this is implemented

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sip Superset Improvement Proposal
Projects
Status: Implemented / Done
Development

No branches or pull requests

5 participants