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.

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

fix(tableau): fixes wrong argument when reauthenticating #12216

Merged
merged 10 commits into from
Dec 24, 2024
52 changes: 37 additions & 15 deletions metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,12 @@
self.parsed_columns = parsed_columns


@dataclass
class SiteIdContentUrl:
site_id: str
site_content_url: str


class TableauSourceReport(StaleEntityRemovalSourceReport):
get_all_datasources_query_failed: bool = False
num_get_datasource_query_failures: int = 0
Expand Down Expand Up @@ -773,7 +779,6 @@
config=self.config,
ctx=self.ctx,
site=site,
site_id=site.id,
report=self.report,
server=self.server,
platform=self.platform,
Expand All @@ -792,8 +797,11 @@
site_source = TableauSiteSource(
config=self.config,
ctx=self.ctx,
site=site,
site_id=self.server.site_id,
site=site
if site
else SiteIdContentUrl(
site_id=self.server.site_id, site_content_url=self.config.site
),
report=self.report,
server=self.server,
platform=self.platform,
Expand Down Expand Up @@ -826,8 +834,7 @@
self,
config: TableauConfig,
ctx: PipelineContext,
site: Optional[SiteItem],
site_id: Optional[str],
site: Union[SiteItem, SiteIdContentUrl],
report: TableauSourceReport,
server: Server,
platform: str,
Expand All @@ -838,13 +845,18 @@
self.ctx: PipelineContext = ctx
self.platform = platform

self.site: Optional[SiteItem] = site
if site_id is not None:
self.site_id: str = site_id
self.site: Optional[SiteItem] = None
if isinstance(site, SiteItem):
self.site = site
assert site.id is not None, "Site ID is required"
self.site_id = site.id
self.site_content_url = site.content_url
elif isinstance(site, SiteIdContentUrl):
self.site = None
self.site_id = site.site_id
self.site_content_url = site.site_content_url

Check warning on line 857 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L855-L857

Added lines #L855 - L857 were not covered by tests
sgomezvillamor marked this conversation as resolved.
Show resolved Hide resolved
else:
assert self.site is not None, "site or site_id is required"
assert self.site.id is not None, "site_id is required when site is provided"
self.site_id = self.site.id
raise AssertionError("site or site id+content_url pair is required")

self.database_tables: Dict[str, DatabaseTable] = {}
self.tableau_stat_registry: Dict[str, UsageStat] = {}
Expand Down Expand Up @@ -898,10 +910,20 @@
# datasets also have the env in the browse path
return f"/{self.config.env.lower()}{self.no_env_browse_prefix}"

def _re_authenticate(self):
# Sign-in again may not be enough because Tableau sometimes caches invalid sessions
# so we need to recreate the Tableau Server object
self.server = self.config.make_tableau_client(self.site_id)
def _re_authenticate(self) -> None:
if self.site_content_url:
assert self.site_content_url is not None, "Site Content URL is required"
sgomezvillamor marked this conversation as resolved.
Show resolved Hide resolved
self.report.info(

Check warning on line 916 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L914-L916

Added lines #L914 - L916 were not covered by tests
f"Re-authenticating to Tableau site='{self.site_content_url}'"
)
# Sign-in again may not be enough because Tableau sometimes caches invalid sessions
# so we need to recreate the Tableau Server object
self.server = self.config.make_tableau_client(self.site_content_url)

Check warning on line 921 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L921

Added line #L921 was not covered by tests
else:
self.report.warning(

Check warning on line 923 in metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py

View check run for this annotation

Codecov / codecov/patch

metadata-ingestion/src/datahub/ingestion/source/tableau/tableau.py#L923

Added line #L923 was not covered by tests
message="Site Content URL is not set. Unable to re-authenticate.",
context=f"site_id={self.site_id}, site={self.site}",
)

@property
def site_content_url(self) -> Optional[str]:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
from datahub.ingestion.run.pipeline import Pipeline, PipelineContext
from datahub.ingestion.source.tableau import tableau_constant as c
from datahub.ingestion.source.tableau.tableau import (
SiteIdContentUrl,
TableauConfig,
TableauProject,
TableauSiteSource,
Expand Down Expand Up @@ -1008,7 +1009,7 @@ def check_lineage_metadata(
config=config,
ctx=context,
platform="tableau",
site=SiteItem(name="Site 1", content_url="site1"),
site=SiteIdContentUrl(site_id="id1", site_content_url="site1"),
site_id="site1",
report=TableauSourceReport(),
server=Server("https://test-tableau-server.com"),
Expand Down Expand Up @@ -1314,7 +1315,6 @@ def test_permission_warning(pytestconfig, tmp_path, mock_datahub_graph):
config=mock.MagicMock(),
ctx=mock.MagicMock(),
site=mock.MagicMock(),
site_id=None,
server=mock_sdk.return_value,
report=reporter,
)
Expand Down Expand Up @@ -1372,7 +1372,6 @@ def test_extract_project_hierarchy(extract_project_hierarchy, allowed_projects):
ctx=context,
platform="tableau",
site=SiteItem(name="Site 1", content_url="site1"),
site_id="site1",
report=TableauSourceReport(),
server=Server("https://test-tableau-server.com"),
)
Expand Down
Loading