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

MS SQL server query runner: improved query for redash/query_runner/mssql.py #7173

Open
gauiPPP opened this issue Sep 20, 2024 · 3 comments
Open

Comments

@gauiPPP
Copy link

gauiPPP commented Sep 20, 2024

CURRENT VERSION:
def _get_tables(self, schema):
query = """
SELECT table_schema, table_name, column_name
FROM INFORMATION_SCHEMA.COLUMNS
WHERE table_schema NOT IN ('guest','INFORMATION_SCHEMA','sys','db_owner','db_accessadmin'
,'db_securityadmin','db_ddladmin','db_backupoperator','db_datareader'
,'db_datawriter','db_denydatareader','db_denydatawriter'
);
"""

causes problems in query execution when there are spaces or special characters in SQL table or column names, probably also schema name.
In addition the column should be sorted in their SQL Server order and not alphabetically, this would improve usability.

SUGGESTED NEW VERSION
(this worked in local test or connection was created, double concats are needed for MS SQL server - BUT in query builder the tables and column still turned out as table name, column name instead of the [table name], [column name] what would have been the input from this query)

def _get_tables(self, schema):
    query = """
          SELECT CONCAT('[',CONCAT(table_schema,']')) as table_schema, CONCAT('[',CONCAT(table_name,']')) as table_name, CONCAT('[',CONCAT(column_name,']')) as column_name
                  FROM INFORMATION_SCHEMA.COLUMNS
                  WHERE table_schema NOT IN ('guest','INFORMATION_SCHEMA','sys','db_owner','db_accessadmin'
                                            ,'db_securityadmin','db_ddladmin','db_backupoperator','db_datareader'
                                            ,'db_datawriter','db_denydatareader','db_denydatawriter'
                                            )
                  Order By
                   INFORMATION_SCHEMA.COLUMNS.TABLE_SCHEMA
                  , INFORMATION_SCHEMA.COLUMNS.TABLE_NAME
                  , INFORMATION_SCHEMA.COLUMNS.ORDINAL_POSITION;
    """

So apparently this also or only an issue with query builder?

@gauiPPP
Copy link
Author

gauiPPP commented Sep 20, 2024

I wrote the new query to one mssql.py within docker folders, apparently there are more so I should test another way.

@gauiPPP
Copy link
Author

gauiPPP commented Sep 20, 2024

I followed the instructions to set up a local development environment and before make and make build I changed the def _get_tables(self, schema) to be the same as in my suggestion
=> running the redash showed [table name] and [column name] as intended and connection to SQL and queries worked. So this should be ok for publishing / production

The sort order was not followed in the query builder, still alphabetical.

@gauiPPP
Copy link
Author

gauiPPP commented Sep 20, 2024

According to instructions first commit then testing :)
I did not run tests before changes so hard to say (without better understanding of redash code) whether or not this error / failure is due to changes in _get_tables(self, schema)

== short test summary info ==
FAILED tests/models/test_alerts.py::TestAlertRenderTemplate::test_render_custom_alert_template - AssertionError: "\n<p[165 chars]ps://htt p://localhost:5001/default/alerts/1\nQ[213 chars]e>\n" != "\n<p[165 chars]ps:///default/aler...
== 1 failed, 877 passed, 1 skipped, 1915 warnings in 177.57s (0:02:57) ==

_________ TestAlertRenderTemplate.test_render_custom_alert_template _________________

self = <tests.models.test_alerts.TestAlertRenderTemplate testMethod=test_render_custom_alert_template>

def test_render_custom_alert_template(self):
    alert = self.create_alert(get_results(1))
    custom_alert = """
    <pre>
    ALERT_STATUS        {{ALERT_STATUS}}
    ALERT_SELECTOR      {{ALERT_SELECTOR}}
    ALERT_CONDITION     {{ALERT_CONDITION}}
    ALERT_THRESHOLD     {{ALERT_THRESHOLD}}
    ALERT_NAME          {{ALERT_NAME}}
    ALERT_URL           {{{ALERT_URL}}}
    QUERY_NAME          {{QUERY_NAME}}
    QUERY_URL           {{{QUERY_URL}}}
    QUERY_RESULT_VALUE  {{QUERY_RESULT_VALUE}}
    QUERY_RESULT_ROWS   {{{QUERY_RESULT_ROWS}}}
    QUERY_RESULT_COLS   {{{QUERY_RESULT_COLS}}}
    </pre>
    """
    expected = """
    <pre>
    ALERT_STATUS        UNKNOWN
    ALERT_SELECTOR      first
    ALERT_CONDITION     equals
    ALERT_THRESHOLD     5
    ALERT_NAME          %s
    ALERT_URL           https:///default/alerts/%d
    QUERY_NAME          Query
    QUERY_URL           https:///default/queries/%d
    QUERY_RESULT_VALUE  1
    QUERY_RESULT_ROWS   [{'foo': 1}]
    QUERY_RESULT_COLS   [{'name': 'foo', 'type': 'STRING'}]
    </pre>
    """ % (
        alert.name,
        alert.id,
        alert.query_id,
    )
    result = alert.render_template(textwrap.dedent(custom_alert))
  self.assertMultiLineEqual(result, textwrap.dedent(expected))

E AssertionError: "\n<p[165 chars]ps://http://localhost:5001/default/alerts/1\nQ[213 chars]e>\n" != "\n<p[165 chars]ps:///default/a lerts/1\nQUERY_NAME Qu[171 chars]e>\n"
E Diff is 648 characters long. Set self.maxDiff to None to see it.

tests/models/test_alerts.py:181: AssertionError

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant