Skip to content

Database dialect is handled in the wrong way #971

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

Closed
misiek1984 opened this issue Jul 10, 2023 · 0 comments · Fixed by #1002
Closed

Database dialect is handled in the wrong way #971

misiek1984 opened this issue Jul 10, 2023 · 0 comments · Fixed by #1002
Assignees
Labels
api: spanner Issues related to the googleapis/python-spanner API.

Comments

@misiek1984
Copy link

misiek1984 commented Jul 10, 2023

Currently CloudSpanner supports GoogleSql and PostgreSql dialects. So in the code there are ifs which change the bahaviour of the library depending on the dialect. For example in Table API there is the following code:

        if self._database.database_dialect == DatabaseDialect.POSTGRESQL:
            results = snapshot.execute_sql(
                _EXISTS_TEMPLATE.format("WHERE TABLE_NAME = $1"),
                params={"p1": self.table_id},
                param_types={"p1": Type(code=TypeCode.STRING)},
            )
        else:
            results = snapshot.execute_sql(
                _EXISTS_TEMPLATE.format("WHERE TABLE_NAME = @table_id"),
                params={"table_id": self.table_id},
                param_types={"table_id": Type(code=TypeCode.STRING)},
            )

The problem is that by default database_dialect is set to 0 == DATABASE_DIALECT_UNSPECIFIED. In this case else part will be executed. By chance it will work for a database that uses GoogleSQL but it will not work for PostgreSql database. In the case of an error a user will see the following misleading message:

 Invalid parameter name: table_id. Expected one of 'p1', 'p2', ..., 'p65535'

To reproduce use this code:

my_table = database.table("my_table")
if my_table.exists():
    print("Table with ID 'my_table' exists.")
else:
    print("Table with ID 'my_table' does not exist.")

Where database is a newly created database using PostgreSql dialect. The fix is to call database.reload() which will populate self._database.database_dialect correctly. However, the error message is very misleading and finding the root couse is very difficult. The situation can be improved if we use else if instead of else and if we raise an error for unknown dialect:

        if self._database.database_dialect == DatabaseDialect.POSTGRESQL:
            results = snapshot.execute_sql(
                _EXISTS_TEMPLATE.format("WHERE TABLE_NAME = $1"),
                params={"p1": self.table_id},
                param_types={"p1": Type(code=TypeCode.STRING)},
            )
        else if self._database.database_dialect == DatabaseDialect.GOOGLESQL:
            results = snapshot.execute_sql(
                _EXISTS_TEMPLATE.format("WHERE TABLE_NAME = @table_id"),
                params={"table_id": self.table_id},
                param_types={"table_id": Type(code=TypeCode.STRING)},
            )
        else :
            raise ValueError("Unknown database dialect. Try to call 'reload' on a database object.")
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/python-spanner API. label Jul 10, 2023
@asthamohta asthamohta linked a pull request Aug 16, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/python-spanner API.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants