fix: don't strip SQL comments in Explore - 2nd try

Here I'm trying to recreate the issue that led to revert #28363 in #28567, likely some sort of trailing comment of some kind. This is DRAFT for now until I can reproduce the issue, write a covering test for it, and address it.

First attempt at creating a problematic virtual dataset was the following ->

superset/db_engine_specs/base.py

@@ -1133,9 +1133,8 @@ def apply_top_to_sql(cls, sql: str, limit: int) -> str:
         cte = None
         sql_remainder = None
         sql = sql.strip(" \t\n;")
-        sql_statement = sqlparse.format(sql, strip_comments=True)
         query_limit: int | None = sql_parse.extract_top_from_query(
-            sql_statement, cls.top_keywords
+            sql, cls.top_keywords
         )
         if not limit:
             final_limit = query_limit
@@ -1144,7 +1143,7 @@ def apply_top_to_sql(cls, sql: str, limit: int) -> str:
         else:
             final_limit = limit
         if not cls.allows_cte_in_subquery:
-            cte, sql_remainder = sql_parse.get_cte_remainder_query(sql_statement)
+            cte, sql_remainder = sql_parse.get_cte_remainder_query(sql)
         if cte:
             str_statement = str(sql_remainder)
             cte = cte + "\n"

superset/models/helpers.py

@@ -1089,7 +1089,7 @@ def get_rendered_sql(
                 _("Virtual dataset query cannot consist of multiple statements")
             )
 
-        sql = script.statements[0].format(comments=False)
+        sql = script.statements[0].format()
         if not sql:
             raise QueryObjectValidationError(_("Virtual dataset query cannot be empty"))
         return sql
@@ -1107,6 +1107,11 @@ def get_from_clause(
         """
 
         from_sql = self.get_rendered_sql(template_processor)
+
+        # TEST this SOLUTION (?)
+        # Add a line break in case last line happens to be a comment
+        # from_sql = from_sql + '\n'
+
         parsed_query = ParsedQuery(from_sql, engine=self.db_engine_spec.engine)
         if not (
             parsed_query.is_unknown()

superset/sqllab/query_render.py

@@ -60,7 +60,6 @@ def render(self, execution_context: SqlJsonExecutionContext) -> str:
 
             parsed_query = ParsedQuery(
                 query_model.sql,
-                strip_comments=True,
                 engine=query_model.database.db_engine_spec.engine,
             )
             rendered_query = sql_template_processor.process_template(

tests/integration_tests/conftest.py

@@ -304,6 +304,7 @@ def virtual_dataset():
             "SELECT 3 as col1, 'd' as col2, 1.3, NULL, '2000-01-04 00:00:00', 4 "
             "UNION ALL "
             "SELECT 4 as col1, 'e' as col2, 1.4, NULL, '2000-01-05 00:00:00', 5 "
+            "\n /* CONTAINS A RANDOM COMMENT */ \n"
             "UNION ALL "
             "SELECT 5 as col1, 'f' as col2, 1.5, NULL, '2000-01-06 00:00:00', 6 "
             "UNION ALL "

tests/integration_tests/core_tests.py

@@ -539,7 +539,9 @@ def test_comments_in_sqlatable_query(self):
             database=get_example_database(),
         )
         rendered_query = str(table.get_from_clause()[0])
-        self.assertEqual(clean_query, rendered_query)
+        assert "comment 1" in rendered_query
+        assert "comment 2" in rendered_query
+        assert "FROM tbl" in rendered_query
 
     def test_slice_payload_no_datasource(self):
         form_data = {

tests/integration_tests/datasource_tests.py

@@ -538,10 +538,12 @@ def test_get_samples(test_client, login_as_admin, virtual_dataset):
     assert "coltypes" in rv2.json["result"]
     assert "data" in rv2.json["result"]
 
-    eager_samples = virtual_dataset.database.get_df(
-        f"select * from ({virtual_dataset.sql}) as tbl"
-        f' limit {app.config["SAMPLES_ROW_LIMIT"]}'
+    sql = (
+        f"select * from ({virtual_dataset.sql}) as tbl "
+        f'limit {app.config["SAMPLES_ROW_LIMIT"]}'
     )
+    eager_samples = virtual_dataset.database.get_df(sql)
+
     # the col3 is Decimal
     eager_samples["col3"] = eager_samples["col3"].apply(float)
     eager_samples = eager_samples.to_dict(orient="records")

tests/integration_tests/sqllab_tests.py

@@ -573,9 +573,9 @@ def test_sql_json_parameter_error(self):
         assert data["status"] == "success"
 
         data = self.run_sql(
-            "SELECT * FROM birth_names WHERE state = '{{ state }}' -- blabblah {{ extra1 }} {{fake.fn()}}\nLIMIT 10",
+            "SELECT * FROM birth_names WHERE state = '{{ state }}' -- blabblah {{ extra1 }}\nLIMIT 10",
             "3",
-            template_params=json.dumps({"state": "CA"}),
+            template_params=json.dumps({"state": "CA", "extra1": "comment"}),
         )
         assert data["status"] == "success"