feat(SIP-95): new endpoint for extra table metadata

[betodealmeida]

SUMMARY

I started working on SIP-95, which introduces catalogs to Superset. The introduction of a new hierarchical level, catalog → schema → table, means that some API endpoints need to be updated to support a new parameter.

One of these API endpoints is used to fetch extra metadata about a given table, and the route looks like this:

/<int:pk>/table_extra/<path:table_name>/<schema_name>/

There are a few problems with the current route:

• table_name is a path, because table names can contain slashes.
• schema_name is often passed as null or undefined, so we need special logic to convert it to None in the backend.
• schema_name should never contain slashes, otherwise the parsing will be wrong.

Because of this, instead of enhancing the existing endpoint to add support for catalogs, I decided to create a new one:

/<int:pk>/table_metadata/extra/?table=t[&schema=s[&catalog=c]]

In this new endpoint, table, schema, and catalog are all passed as query parameters. This allows slashes in all of them, and prevents polluting logs with confusing URLs such as /api/v1/databses/1/table_extra/my_table/undefined/. Only table is required, the other two are optional.

The API calls a new method get_extra_table_metadata in the corresponding DB engine spec. The new method takes a Table object, which contains a name and optional schema/catalog. To prevent breaking any 3rd-party DB engine specs the new method will issue a warning and call extra_table_metadata if it exists and no catalog was specified.

Note that while the old endpoint is not removed in this PR, to prevent breaking changes, the frontend code was updated to use the new endpoint.

In the future, we should remove the old endpoint and also the extra_table_metadata method. @michael-s-molina, what do you think of this approach of introducing a new API? Should we mark it as deprecated now and remove it in the next release, or should we mark is as deprecated in 5.0 and remove it in 6.0?

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A

TESTING INSTRUCTIONS

I updated existing tests and added new unit tests.

ADDITIONAL INFORMATION

• Has associated issue: [SIP-95] Proposal for Catalog Support in SQL Lab #22862
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[michael-s-molina]

Should we mark it as deprecated now and remove it in the next release, or should we mark is as deprecated in 5.0 and remove it in 6.0?

You can mark it as deprecated now. Please add a proposal here to remove the old endpoint in 5.0.

In this new endpoint, table, schema, and catalog are all passed as query parameters. This allows slashes in all of them, and prevents polluting logs with confusing URLs such as /api/v1/databses/1/table_extra/my_table/undefined/.

Nesting resources or not is an architectural decision, with trade-offs, that we need to make consistently in our API. Because we built the API one endpoint at a time, we don't have consistency in nomenclature, response types, params, etc. I'm hoping to fix all these problems with v2 where we would analyze all existing endpoints and propose a consistent API in a SIP. For that reason, any pattern you choose now is acceptable and will be reviewed in v2.

[betodealmeida]

Thanks, @michael-s-molina!

Nesting resources or not is an architectural decision, with trade-offs, that we need to make consistently in our API. Because we built the API one endpoint at a time, we don't have consistency in nomenclature, response types, params, etc. I'm hoping to fix all these problems with v2 where we would analyze all existing endpoints and propose a consistent API in a SIP. For that reason, any pattern you choose now is acceptable and will be reviewed in v2.

Oh, I'm not against nesting resources in general. The problem here is that table names can contain slashes, and I think we should assume that schemas and catalogs can too. Imagine we have this route:

/some/api/<path:catalog>/<path:schema>/<path:table>/

And we have a table with slashes in the catalog, schema, and name, which we escape with %2F when doing a request:

/some/api/my%2Fcatalog/my%2Fschema/my%2Ftable/

What the Flask router sees in the request is:

/some/api/my/catalog/my/schema/my/table/

Which gets mapped to:

• catalog: my
• schema: catalog
• table: my/schema/my/table

So we'd have to double escape the slashes:

/some/api/my%252Fcatalog/my%252Fschema/my%252Ftable/

And the route would have unescape them back. For cases like this I think it's easier to pass the parameters as query arguments:

/some/api/?table=my/table&schema=my/schema&catalog=my/catalog

While longer, I think it's easier to read. And when some of those parameters are missing it gets better:

/some/api/?table=my/table

vs.

/some/api/undefined/undefined/my%252Ftable/

So something to think about when designing v2.

[michael-s-molina]

Oh, I'm not against nesting resources in general. The problem here is that table names can contain slashes, and I think we should assume that schemas and catalogs can too.
So something to think about when designing v2.

Yep! That's indeed a requirement we should consider when designing v2.

[john-bodley]

Should we be passing a Table instance everywhere?

[betodealmeida]

We should! I'm doing that in the next PR! 😊

[dpgaspar]

nice!

[dpgaspar]

optional:
do:

if not hasattr(cls, "extra_table_metadata"):
    return {}
...

to reduce nesting

[betodealmeida]

Nice, I'll fix it in the next PR!