-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
feat(dataset API): Add parameter to optionally render Jinja macros in API response #30721
base: master
Are you sure you want to change the base?
Conversation
item: Optional[SqlaTable] = self.datamodel.get( | ||
pk, | ||
self._base_filters, | ||
self.show_select_columns, | ||
self.show_outer_default_load, | ||
) | ||
if not item: | ||
return self.response_404() | ||
|
||
response: dict[str, Any] = {} | ||
args = kwargs.get("rison", {}) | ||
select_cols = args.get(API_SELECT_COLUMNS_RIS_KEY, []) | ||
pruned_select_cols = [col for col in select_cols if col in self.show_columns] | ||
self.set_response_key_mappings( | ||
response, | ||
self.get, | ||
args, | ||
**{API_SELECT_COLUMNS_RIS_KEY: pruned_select_cols}, | ||
) | ||
if pruned_select_cols: | ||
show_model_schema = self.model2schemaconverter.convert(pruned_select_cols) | ||
else: | ||
show_model_schema = self.show_model_schema | ||
|
||
response["id"] = pk | ||
response[API_RESULT_RES_KEY] = show_model_schema.dump(item, many=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is FAB's default implementation for get_headless
- ref.
I decided to not simply do self.get_headless()
and then modify the response for two reasons:
get_headless
returns a response object, so while we could just modify theresponse.json
it feels a little hacky.- Superset's
get_headless
implementation includes a call tosend_stats_metrics
(ref), which wouldn't include the full duration of the API call as the response would be further modified by this method before concluding.
I also thought about just overwriting FAB's pre_get
method which is available in case the response needs to be modified as part of get_headless
, but we need the Database
associated with this dataset, and if the user chooses to not include the database.id
in the response then pre_get
wouldn't have this info.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -18,11 +18,16 @@ | |||
import logging | |||
from datetime import datetime | |||
from io import BytesIO | |||
from typing import Any | |||
from typing import Any, Optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hint: you can do from __future__ import annotations
and then use a | None
instead of Optional[a]
.
SUMMARY
This PR adds support for optionally rendering Jinja macros in the API response when getting a dataset. The motivation for this feature is that external tools might be fetching dataset metadata via the API to define lineage/etc, and in case Jinja is used it's not possible to fully parse the SQL query to identify dependencies/etc. An example query:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before
After
TESTING INSTRUCTIONS
Testing added.
ADDITIONAL INFORMATION