Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(ingest/tableau): prevent empty site content urls #11057

Merged
merged 6 commits into from
Aug 7, 2024
Merged

fix(ingest/tableau): prevent empty site content urls #11057

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3f20cc6
fix(ingest/tableau): prevent empty site content urls
hsheth2 Jul 31, 2024
55de7a1
Merge branch 'master' into tableau-site-url
hsheth2 Jul 31, 2024
9c74076
Update metadata-ingestion/src/datahub/ingestion/source/tableau.py
hsheth2 Jul 31, 2024
ddc66f1
Merge branch 'master' into tableau-site-url
hsheth2 Jul 31, 2024
e1261cd
Merge branch 'master' into tableau-site-url
hsheth2 Aug 2, 2024
e150791
fix lint
hsheth2 Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions metadata-ingestion/src/datahub/ingestion/source/tableau.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,12 @@ def _re_authenticate(self):
] = self.config.get_tableau_auth(self.site.content_url)
self.server.auth.sign_in(tableau_auth)

@property
def site_content_url(self) -> Optional[str]:
if self.site and self.site.content_url:
return self.site.content_url
return None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like we always use this in a ternary. Could we just return "" instead and avoid the ternary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not really - sometimes we need /site/{self.site_content_url}, but sometimes the prefix is /t/ instead of site


def _populate_usage_stat_registry(self) -> None:
if self.server is None:
return
Expand Down Expand Up @@ -2524,7 +2530,9 @@ def emit_sheets_as_charts(
last_modified = self.get_last_modified(creator, created_at, updated_at)

if sheet.get(c.PATH):
site_part = f"/site/{self.site.content_url}" if self.site else ""
site_part = (
f"/site/{self.site_content_url}" if self.site_content_url else ""
)
sheet_external_url = (
f"{self.config.connect_uri}/#{site_part}/views/{sheet.get(c.PATH)}"
)
Expand All @@ -2535,7 +2543,7 @@ def emit_sheets_as_charts(
and sheet[c.CONTAINED_IN_DASHBOARDS][0].get(c.PATH)
):
# sheet contained in dashboard
site_part = f"/t/{self.site.content_url}" if self.site else ""
site_part = f"/t/{self.site_content_url}" if self.site_content_url else ""
dashboard_path = sheet[c.CONTAINED_IN_DASHBOARDS][0][c.PATH]
sheet_external_url = f"{self.config.connect_uri}{site_part}/authoring/{dashboard_path}/{quote(sheet.get(c.NAME, ''), safe='')}"
else:
Expand Down Expand Up @@ -2667,7 +2675,7 @@ def emit_workbook_as_container(self, workbook: Dict) -> Iterable[MetadataWorkUni
else None
)

site_part = f"/site/{self.site.content_url}" if self.site else ""
site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
workbook_uri = workbook.get("uri")
workbook_part = (
Comment on lines +2678 to 2680
Copy link
Contributor

Choose a reason for hiding this comment

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

Simplify URL construction logic.

The logic for constructing workbook_external_url can be simplified by combining the if conditions.

- site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
- workbook_uri = workbook.get("uri")
- workbook_part = workbook_uri[workbook_uri.index("/workbooks/") :] if workbook_uri else None
+ workbook_uri = workbook.get("uri")
+ if workbook_uri:
+     site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
+     workbook_part = workbook_uri[workbook_uri.index("/workbooks/") :]
+     workbook_external_url = f"{self.config.connect_uri}/#{site_part}{workbook_part}"
+ else:
+     workbook_external_url = None
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
workbook_uri = workbook.get("uri")
workbook_part = (
workbook_uri = workbook.get("uri")
if workbook_uri:
site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
workbook_part = workbook_uri[workbook_uri.index("/workbooks/") :]
workbook_external_url = f"{self.config.connect_uri}/#{site_part}{workbook_part}"
else:
workbook_external_url = None

workbook_uri[workbook_uri.index("/workbooks/") :] if workbook_uri else None
Expand Down Expand Up @@ -2826,7 +2834,7 @@ def emit_dashboard(
updated_at = dashboard.get(c.UPDATED_AT, datetime.now())
last_modified = self.get_last_modified(creator, created_at, updated_at)

site_part = f"/site/{self.site.content_url}" if self.site else ""
site_part = f"/site/{self.site_content_url}" if self.site_content_url else ""
dashboard_external_url = (
f"{self.config.connect_uri}/#{site_part}/views/{dashboard.get(c.PATH, '')}"
)
Expand Down
Loading