-
Notifications
You must be signed in to change notification settings - Fork 16.6k
feat(dashboards): Add config to filter implicit tags in list API #36246
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(dashboards): Add config to filter implicit tags in list API #36246
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #36246 +/- ##
===========================================
+ Coverage 0 68.15% +68.15%
===========================================
Files 0 633 +633
Lines 0 46512 +46512
Branches 0 5043 +5043
===========================================
+ Hits 0 31698 +31698
- Misses 0 13547 +13547
- Partials 0 1267 +1267
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
91ae62b to
94236df
Compare
dpgaspar
left a comment
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.
LGTM. We should definitely filter out non custom tags as the default behaviour.
Is it still possible to get into the 6.X breaking change collection with something like that @sadpandajoe ?
c966dd4
into
apache:master
) (cherry picked from commit c966dd4)
) (cherry picked from commit c966dd4)
…che#36246) (cherry picked from commit c966dd4)
…che#36246) (cherry picked from commit c966dd4)
SUMMARY
Noticed our Dashboard List API is returning ALL the implicit tags in the List endpoint, which is damaging the performance of such API. These tags are pure metadata, and not used for any security process in our app, what's more, even the UI is filtering those and only rendering
customones.This PR introduces a new config that by default respect the current behavior so nothing gets changed inadvertently, this new config will make FAB filter at the JOIN level which tags to include as part of the response. This new config is read by a new Mixin added to the
DashboardRestApi.What it does? It creates a new relationship called
custom_tagsthat is the one filtering by tag type = custom at the JOIN level, and only uses this relationship when the new config is True, it leverages the fact that FAB uses list_columns to create those joins automatically. On the frontend, the change is transparent as thecustom_tagsget transformed totagson those list endpoints using the mixin.Why a config and not a Feature Flag? The relationship is created at the model and manipulating the model is not something you can do at runtime (request), so that's why I'm using a config.
Why at the JOIN level? Because filtering at python level with for example a callable would not help on query performance, only on response size.
Why a Mixin? We could consider reusing this mixin on other parts where tags are returned by default like
ChartRestApietc later on.Does this affect current functionality? No, the UI works exactly the same as before, you can list resources and see the tags associated with them, you can edit tags, you can filter content by tag etc.
At some point we would like to evaluate how to get rid of those implicit tag altogether in these APIs, but that major effort would be out of the scope of this initial optimization.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Default behavior without filtering:

When the config is enabled:

TESTING INSTRUCTIONS
ADDITIONAL INFORMATION