feat(dataset API): Add parameter to optionally render Jinja macros in API response #30721
+197
−2
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Vitor-Avila
commented
Oct 25, 2024
Comment on lines
+1120
to
+1145
| 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.
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_headlessreturns a response object, so while we could just modify theresponse.jsonit feels a little hacky.- Superset's
get_headlessimplementation 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.
betodealmeida
approved these changes
Oct 25, 2024
| @@ -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.
Hint: you can do from __future__ import annotations and then use a | None instead of Optional[a].
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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