changed pg.py has_privileges function to include special characters a…

[sarvesh68]

Changed pg.py has_privileges function to include special characters and added unit tests to test_pg.py

What type of PR is this?

• Bug Fix

Description

According to issue #7565 : When using PostgreSQL with tables containing mixed-case (camelCase) names, the automatic schema discovery fails with permission errors. The issue occurs in the system query that checks table privileges.

List of tables:

Using previous query for tables names with special characters/whitespaces/camelCase:

It fails to return the proper results:
This is due to the fact that: has_table_privilege requires properly quoted identifiers when names contain uppercase letters or special characters.

Proposed fix (as suggested by the one raised in the issue):
From pg.py:
Add quote_ident() function to the query to properly handle the special identifiers for : materialized views as well as other tables (Original query involves join operation on the two of them)

How is this tested?

• Unit tests (pytest, jest)
• Manually

Unit testing using pytest:

• Added the test_build_schema_with_data_types_with_special_table_names() function in the test_pg.py File
• Mocked query results for tables and materialized views (returned by the run_query() function in pg.py
• Invoked the build_schema() function to return the schema formatted data and compared the column data

Unit test results:
docker compose exec -w /app server pytest -s tests/query_runner/test_pg.py
3 passed

Integration + Manual tests

• Created tables and materialized views with special characters/blank spaces and camelCase in postgres

• Created PostgreSQL Object

• Ran query with changes and returned results using run_query function : run_query(queryunion,user=None)

• Verified structure of returned tuple:

• Passed above query results to build_schema() function : build_schema(results, schema)

• Verified schema output after build_schema() function (as tested in the unittests)

Related Tickets & Documents

## Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[eradman]

Tested, this change works!

Run make format next

[yoshiokatsuneo]

@sarvesh68

I think the test( test_build_schema_with_data_types_with_special_table_names ), and manual testing is not actually testing the feature, and not related to this PR.

Just my curious. Are you using AI agent to make the PR description and the code and the test case and the manual testing ?

Thank you.

[sarvesh68]

@yoshiokatsuneo

No I have not used any AI agents for the same.
Coming to your point about the testing not being related to the feature:

The query is present in the "_get_tables(self, schema)" function in pg.py

This query is then run in the "run_query(query, None)" function (inside of "_get_definitions(schema, query)" function) and a schema is built corresponding to the query results. (Have tested whether the query returns result without throwing up an error manually as well)

As the building of the schema using the "build_schema(results, schema)" function is downstream to the query : I have tested it accordingly to see if it is being built properly for the special cases we have namely : camelCase/special characters/blank spaces in table or materialized views names and they are being passed as an argument to the
test_build_schema_with_data_types_with_special_table_names function in the unittest.

I have also created a separate integration test script to test the functionality of the above portion after the query change (locally and not included in the PR)

Thanks

[yoshiokatsuneo]

@sarvesh68

Thanks !

Is the test test_build_schema_with_data_types_with_special_table_names failed before the PR ?

[yoshiokatsuneo]

@sarvesh68

For about "Integration + Manual tests", I'm not sure how you are manually testing.

Created tables and materialized views with special characters/blank spaces and camelCase in postgres

I can suggest to use text instead of image so that people can easily copy and paste.
And, I cannot see anything related to the materialized view described in the sentence.

Ran query with changes and returned results using run_query function : run_query(queryunion,user=None)

I'm not sure what is the queryunion.

Thank you.

[eradman]

I think the test( test_build_schema_with_data_types_with_special_table_names ), and manual testing is not actually testing the feature, and not related to this PR.

Indeed this test passes even without the change in redash/query_runner/pg.py

[sarvesh68]

@yoshiokatsuneo @eradman

I would like to point out that the rows being passed inside the "test_build_schema_with_data_types_with_special_table_names" function to "build_schema(results, schema)" : are the results of the query : which wouldn’t have been returned for the special names(camelCase and whitespaces) that we discussed, BEFORE the PR (would have thrown an error and not returned the results)

AFTER the PR the query does return results (with the special names) which have been mocked as input to the "build_schema(results, schema)" inside of "test_build_schema_with_data_types_with_special_table_names " function.

Would the "test_build_schema_with_data_types_with_special_table_names " function pass the test before the PR? : maybe yes : but as these special names would have to be handled by the build_schema function only AFTER the PR, hence I have added the corresponding tests as it improves coverage and handles more scenarios.

Queryunion is just a variable name : as the query involves union of two parts of the query : one that returns all the tables and one that returns the materialized views (WHERE c.relkind = 'm') (as you can see in pg.py) (Hence quote_ident has been added to both the parts of the query on either side of the UNION operator)

I was also able to test it for materialized views by creating the same using special names and subsequently running the query that is present inside pg.py (as done for tables):
For eg:
CREATE MATERIALIZED VIEW "org1Tablemview" AS
SELECT * FROM "org1Table";
CREATE MATERIALIZED VIEW "orgTablemview" AS
SELECT * FROM "orgTable";

Thanks for the inputs!

[yoshiokatsuneo]

@sarvesh68

Thank you.

It looks that the changing on the query looks good.

And, I see that the test is not for testing the query but testing behavior after the query. I'm not so sure about the value of the test, but it is working.

[eradman]

I would say let's just remove the new test. We have validated this change manually, and it is an important fix.

[yoshiokatsuneo]

@eradman @sarvesh68 Thank you!