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: move dynamic schema out of base Postgres class #23868

Merged
merged 3 commits into from
May 1, 2023
Merged
Changes from 2 commits
Commits
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
96 changes: 65 additions & 31 deletions superset/db_engine_specs/postgres.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,6 @@ class PostgresBaseEngineSpec(BaseEngineSpec):
engine = ""
engine_name = "PostgreSQL"

supports_dynamic_schema = True
supports_catalog = True

_time_grain_expressions = {
Expand Down Expand Up @@ -167,25 +166,6 @@ class PostgresBaseEngineSpec(BaseEngineSpec):
),
}

@classmethod
def adjust_engine_params(
cls,
uri: URL,
connect_args: Dict[str, Any],
catalog: Optional[str] = None,
schema: Optional[str] = None,
) -> Tuple[URL, Dict[str, Any]]:
if not schema:
return uri, connect_args

options = parse_options(connect_args)
options["search_path"] = schema
connect_args["options"] = " ".join(
f"-c{key}={value}" for key, value in options.items()
)

return uri, connect_args

@classmethod
def get_schema_from_engine_params(
cls,
Expand All @@ -205,17 +185,19 @@ def get_schema_from_engine_params(
to determine the schema for a non-qualified table in a query. In cases like
that we raise an exception.
"""
options = parse_options(connect_args)
if search_path := options.get("search_path"):
schemas = search_path.split(",")
if len(schemas) > 1:
raise Exception(
"Multiple schemas are configured in the search path, which means "
"Superset is unable to determine the schema of unqualified table "
"names and enforce permissions."
)
return schemas[0]

options = re.split(r"-c\s?", connect_args.get("options", ""))
Copy link
Member

Choose a reason for hiding this comment

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

I think we can actually get rid of the get_schema_from_engine_params method here, since Redshift doesn't support passing search_path as far as I could see.

for option in options:
if "=" not in option:
continue
key, value = option.strip().split("=", 1)
if key.strip() == "search_path":
if "," in value:
raise Exception(
"Multiple schemas are configured in the search path, which means "
"Superset is unable to determine the schema of unqualified table "
"names and enforce permissions."
)
return value.strip()
return None

@classmethod
Expand All @@ -234,6 +216,7 @@ def epoch_to_dttm(cls) -> str:
class PostgresEngineSpec(PostgresBaseEngineSpec, BasicParametersMixin):
engine = "postgresql"
engine_aliases = {"postgres"}
supports_dynamic_schema = True

default_driver = "psycopg2"
sqlalchemy_uri_placeholder = (
Expand Down Expand Up @@ -268,6 +251,57 @@ class PostgresEngineSpec(PostgresBaseEngineSpec, BasicParametersMixin):
),
)

@classmethod
def get_schema_from_engine_params(
cls,
sqlalchemy_uri: URL,
connect_args: Dict[str, Any],
) -> Optional[str]:
"""
Return the configured schema.

While Postgres doesn't support connecting directly to a given schema, it allows
users to specify a "search path" that is used to resolve non-qualified table
names; this can be specified in the database ``connect_args``.

One important detail is that the search path can be a comma separated list of
schemas. While this is supported by the SQLAlchemy dialect, it shouldn't be used
in Superset because it breaks schema-level permissions, since it's impossible
to determine the schema for a non-qualified table in a query. In cases like
that we raise an exception.
"""
options = parse_options(connect_args)
if search_path := options.get("search_path"):
schemas = search_path.split(",")
if len(schemas) > 1:
raise Exception(
"Multiple schemas are configured in the search path, which means "
"Superset is unable to determine the schema of unqualified table "
"names and enforce permissions."
)
return schemas[0]

return None

@classmethod
def adjust_engine_params(
cls,
uri: URL,
connect_args: Dict[str, Any],
catalog: Optional[str] = None,
schema: Optional[str] = None,
) -> Tuple[URL, Dict[str, Any]]:
if not schema:
return uri, connect_args

options = parse_options(connect_args)
options["search_path"] = schema
connect_args["options"] = " ".join(
f"-c{key}={value}" for key, value in options.items()
)

return uri, connect_args

@classmethod
def get_allow_cost_estimate(cls, extra: Dict[str, Any]) -> bool:
return True
Expand Down