sha174n commented on PR #38452: URL: https://github.com/apache/superset/pull/38452#issuecomment-4129563548
> This is looking great. > > Final comments: > > 1. MERGE not covered in DML table extraction - extract_tables_from_statement adds explicit handling for exp.Update, exp.Delete, and exp.Insert but not exp.Merge. A MERGE INTO protected_table USING ... > would have an empty table set and bypass the DML blocking check. The is_mutating() method correctly detects MERGE, but statement.tables would be empty so get_predicates_for_table would never be called > for it. > 2. exp.Copy not in is_mutating command check - The PR adds MERGE, UPSERT, REPLACE, COPY, TRUNCATE to the exp.Command name check, but COPY can also parse as exp.Copy (a dedicated AST node), which isn't > in the mutating_nodes tuple. > 3. test_rls_allows_mutation_on_unprotected_table is a false-positive test - It catches all non-SupersetSecurityException exceptions and passes, meaning it would pass even if the code crashes before > reaching the RLS check. This test doesn't actually verify the unprotected path works correctly. > 4. _process_rls_clause could return empty parens - If _process_select_expression returns an empty string (which is falsy), the if not processed check would fall through to the template_processor path. > But if it returns None (as mocked in tests), the fallback works. Worth verifying the actual return values. Thanks for the detailed review! 1. extract_tables_from_statement handles exp.Merge explicitly at parse.py:1496 — the target table is added to sources alongside the traversed ones. 2. exp.Copy is in mutating_nodes at parse.py:712, so it's caught by the AST walk in addition to the command name check. 3. Valid catch — also strengthened test_validate_adhoc_subquery_preserves_jinja_on_non_predicate in the latest commit with return_value=True and assert_called_once(). 4. Could you point to the specific location you have in mind for _process_rls_clause? We couldn't trace the pattern you described in validate_adhoc_subquery or the RLS utilities. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
