feat: add Dataset tagging import/update api #22304

cccs-RyanK · 2022-12-01T15:56:34Z

SUMMARY

This work is related to a previous PR which added the ability to tag datasets in the backend. This PR adds the support tobe able to add tags to datasets via the API.

Currently there is an ongoing PR to add tags to the front-end.

It is behind the TAGGING_SYSTEM feature flag

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Tests added to tests/integration_tests/datasets/api_tests.py

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags: TAGGING_SYSTEM
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

codecov · 2022-12-01T16:04:06Z

Codecov Report

Merging #22304 (f2fda0b) into master (c09b8c6) will decrease coverage by 0.31%.
The diff coverage is 90.00%.

❗ Current head f2fda0b differs from pull request most recent head 5eb40ce. Consider uploading reports for the commit 5eb40ce to get more accurate results

@@            Coverage Diff             @@
##           master   #22304      +/-   ##
==========================================
- Coverage   67.18%   66.87%   -0.31%     
==========================================
  Files        1903     1844      -59     
  Lines       74275    70433    -3842     
  Branches     8110     7670     -440     
==========================================
- Hits        49898    47099    -2799     
+ Misses      22265    21346     -919     
+ Partials     2112     1988     -124

Flag	Coverage Δ
hive	`52.48% <32.50%> (?)`
mysql	`78.00% <90.00%> (-1.35%)`	⬇️
postgres	`78.06% <90.00%> (-1.36%)`	⬇️
presto	`52.37% <32.50%> (?)`
python	`81.26% <90.00%> (+1.71%)`	⬆️
sqlite	`76.41% <47.50%> (-1.52%)`	⬇️
unit	`50.88% <40.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/tags/utils.py	`82.92% <82.92%> (ø)`
superset/datasets/commands/importers/v1/utils.py	`63.00% <93.75%> (+9.07%)`	⬆️
superset/datasets/dao.py	`94.80% <100.00%> (+3.50%)`	⬆️
superset/datasets/schemas.py	`97.43% <100.00%> (+1.20%)`	⬆️
superset/tags/exceptions.py	`100.00% <100.00%> (+20.00%)`	⬆️
superset/tags/models.py	`93.79% <100.00%> (+0.35%)`	⬆️

... and 1084 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Antonio-RiveroMartnez · 2023-01-19T19:00:19Z

superset/datasets/commands/importers/v1/utils.py

@@ -148,6 +156,26 @@ def import_dataset(
    return dataset


+def extract_tags(config: Dict[str, Any]) -> Tuple[Dict[str, Any], Optional[List[str]]]:
+    tags = None
+    if "tags" in config.keys() and feature_flag_manager.is_feature_enabled(


I see a broader usage in our app of is_feature_enabled coming from this import directly from superset import is_feature_enabled, any reason why not using that instead?

Antonio-RiveroMartnez · 2023-01-19T19:01:30Z

superset/datasets/dao.py

@@ -167,6 +169,11 @@ def update(
        if "metrics" in properties:
            cls.update_metrics(model, properties.pop("metrics"), commit=commit)

+        if "tags" in properties and feature_flag_manager.is_feature_enabled(


same as above

Antonio-RiveroMartnez · 2023-01-19T19:08:12Z

superset/datasets/commands/importers/v1/utils.py

+    return config, tags
+
+
+def import_tags(dataset: SqlaTable, existing: bool, tags: Optional[List[str]]) -> None:


Looking at this method, it's only doing something if tags has value in it, so why is tags Optional then? Can we just mark it as required and not call this method if tag if empty for example? or do you see a reason to keep it as is?

hughhhh · 2023-01-19T21:00:52Z

superset/datasets/commands/importers/v1/utils.py

@@ -148,6 +156,26 @@ def import_dataset(
    return dataset


+def extract_tags(config: Dict[str, Any]) -> Tuple[Dict[str, Any], Optional[List[str]]]:


can we get a unit test for this util function

hughhhh · 2023-01-19T21:03:10Z

superset/datasets/commands/importers/v1/utils.py

+    return config, tags
+
+
+def import_tags(dataset: SqlaTable, existing: bool, tags: Optional[List[str]]) -> None:


can we get unit test for this function

hughhhh · 2023-01-19T21:04:26Z

superset/tags/utils.py

+from superset.tags.models import get_tag, ObjectTypes, Tag, TaggedObject, TagTypes
+
+
+def validate_custom_tags(


hughhhh · 2023-01-19T21:04:34Z

superset/tags/utils.py

+
+
+def add_custom_object_tags(
+    tags: List[str],


Co-authored-by: Aleksey Karpov <al.karpov@okko.tv>

…pache#24404)

…tils/client-ws-app (apache#24501) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

… in /superset-websocket (apache#24522) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…websocket (apache#24524) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…4546)

Co-authored-by: Justin Park <justinpark@apache.org>

…#21255)

…ataset-Tagging-Import/Update-API

geido · 2024-03-08T18:58:35Z

Hey @cccs-RyanK I moved this to a draft as it seems it requires a rebase.

pull-request-size bot added the size/XL label Dec 1, 2022

rusackas requested review from lyndsiWilliams, eschutho and villebro December 1, 2022 20:51

pull-request-size bot added size/L size/XL and removed size/XL size/L labels Dec 2, 2022

Antonio-RiveroMartnez reviewed Jan 19, 2023

View reviewed changes

hughhhh reviewed Jan 19, 2023

View reviewed changes

superset/tags/utils.py Outdated

def add_custom_object_tags(

tags: List[str],

Copy link

Member

hughhhh Jan 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

rusackas and others added 14 commits June 29, 2023 12:01

docs: updating images and markdown (apache#24462)

103e4b5

build(docker): add dockerize image (apache#24534)

0cd8b66

Co-authored-by: Aleksey Karpov <al.karpov@okko.tv>

chore: deprecate FLASK_ENV and improve conf.ENVIRONMENT_TAG_CONFIG (a…

0acde64

…pache#24404)

build(deps): bump semver from 7.3.8 to 7.5.3 in /superset-websocket/u…

3352dbc

…tils/client-ws-app (apache#24501) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

build(deps-dev): bump @typescript-eslint/parser from 5.60.0 to 5.60.1…

076de57

… in /superset-websocket (apache#24522) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

build(deps-dev): bump @types/node from 20.3.1 to 20.3.2 in /superset-…

160cbd1

…websocket (apache#24524) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

build(deps): bump tox from 4.6.1 to 4.6.3 in /requirements (apache#24452

cc686e4

) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

fix: Error when searching for a dashboard in the chart list (apache#2…

6524166

…4546)

feat: add description column to Tags (apache#24553)

d5dc107

fix(sqllab): Add threshold for checking inactive queries (apache#24536)

b8096c4

Co-authored-by: Justin Park <justinpark@apache.org>

feat: customize screenshot width for alerts/reports (apache#24547)

8d6c1aa

chore: better styling alerts/reports modal (apache#24558)

c00b335

feat: screenshot reports with custom width (apache#24549)

6cbc3eb

fix: docker compose refusing to start (apache#24565)

6c0d4b7

dpgaspar and others added 8 commits August 23, 2023 09:31

fix: dataset safe URL for explore_url (apache#24686)

9c4fe34

chore: Organizes the files of the ReportModal feature (apache#25046)

2578da3

fixing pylint exception

63fd2d2

fix: Ensure SQLAlchemy sessions are closed (apache#25031)

6cc675d

fix(cli): import-directory is missing the application context (apache…

7db9161

…#21255)

fix: small fixes for the meta DB (apache#25067)

7a0aa58

fix(table): condition formatting can't formate 0 values (apache#24008)

b174032

Merge branch 'master' of github.com:CybercentreCanada/superset into D…

0505f88

…ataset-Tagging-Import/Update-API

cccs-rc force-pushed the Dataset-Tagging-Import/Update-API branch from 4e2b091 to 0505f88 Compare March 6, 2024 19:09

cccs-RyanK requested review from rusackas, geido, betodealmeida, nytai, mistercrunch, craig-rueda, john-bodley, kgabryje, dpgaspar, jinghua-qa and michael-s-molina as code owners March 6, 2024 19:09

github-actions bot added doc Namespace | Anything related to documentation embedded plugins dependencies:npm github_actions Pull requests that update GitHub Actions code packages dependencies:python labels Mar 6, 2024

geido marked this pull request as draft March 8, 2024 18:57

john-bodley added the review:draft label Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add Dataset tagging import/update api #22304

feat: add Dataset tagging import/update api #22304

cccs-RyanK commented Dec 1, 2022

codecov bot commented Dec 1, 2022 •

edited

Antonio-RiveroMartnez Jan 19, 2023

Antonio-RiveroMartnez Jan 19, 2023

Antonio-RiveroMartnez Jan 19, 2023 •

edited

hughhhh Jan 19, 2023

hughhhh Jan 19, 2023

hughhhh Jan 19, 2023

hughhhh Jan 19, 2023

geido commented Mar 8, 2024

		return config, tags


		def import_tags(dataset: SqlaTable, existing: bool, tags: Optional[List[str]]) -> None:

		@@ -148,6 +156,26 @@ def import_dataset(
		return dataset


		def extract_tags(config: Dict[str, Any]) -> Tuple[Dict[str, Any], Optional[List[str]]]:

		from superset.tags.models import get_tag, ObjectTypes, Tag, TaggedObject, TagTypes


		def validate_custom_tags(

feat: add Dataset tagging import/update api #22304

Are you sure you want to change the base?

feat: add Dataset tagging import/update api #22304

Conversation

cccs-RyanK commented Dec 1, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Dec 1, 2022 • edited

Codecov Report

Antonio-RiveroMartnez Jan 19, 2023

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jan 19, 2023

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jan 19, 2023 • edited

Choose a reason for hiding this comment

hughhhh Jan 19, 2023

Choose a reason for hiding this comment

hughhhh Jan 19, 2023

Choose a reason for hiding this comment

hughhhh Jan 19, 2023

Choose a reason for hiding this comment

hughhhh Jan 19, 2023

Choose a reason for hiding this comment

geido commented Mar 8, 2024

codecov bot commented Dec 1, 2022 •

edited

Antonio-RiveroMartnez Jan 19, 2023 •

edited