✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

✅ PR preview is ready!

Summary

This PR fixes a caching bug in SnowflakeConnection.query() where the params argument was captured as a closure variable instead of being passed as an explicit parameter to the inner _query function. This caused different parameter values to incorrectly share the same cache entry, returning stale results when queries were executed with different parameters.

The fix makes params an explicit parameter of the _query function, ensuring st.cache_data includes it in the cache key. This is consistent with how SQLConnection.query() handles the same pattern.

Code Quality

The code change is minimal, focused, and correct:

Changes in lib/streamlit/connections/snowflake_connection.py:

1. Line 138: The _query inner function signature changed from:

   def _query(sql: str) -> DataFrame:

   to:

   def _query(sql: str, params: Any = None) -> DataFrame:

2. Line 156: The function call changed from:

   return _query(sql)

   to:

   return _query(sql, params)

This pattern is now consistent with SQLConnection.query() (see lines 313-319 and 348-354 in sql_connection.py), where params is explicitly passed to the inner _query function to ensure proper cache key calculation.

The fix correctly addresses the closure variable capture issue - previously, params was accessed from the outer scope inside _query, meaning @st.cache_data would not include it in the cache key calculation.

Test Coverage

The new test test_query_caches_separately_for_different_params adequately covers the fix:

Strengths:

• Tests that different params values create separate cache entries
• Tests that identical params values correctly hit the cache
• Uses a unique connection name (my_snowflake_connection_params) to avoid cache interference with other tests
• Includes a docstring explaining the test purpose
• Follows the existing test patterns in the file

The test logic:

1. Calls query() with params=["active"] - cache miss, executes query
2. Calls query() with params=["inactive"] - cache miss, executes query
3. Calls query() with params=["active"] again - cache hit
4. Calls query() with params=["inactive"] again - cache hit
5. Asserts cursor.call_count == 2 and execute.call_count == 2

This implicitly includes a negative assertion: the test verifies that same params DO hit the cache (2 calls, not 4).

Backwards Compatibility

This change is fully backwards compatible:

• The params parameter in _query has a default value of None, matching the original behavior when params is not specified
• The external API of SnowflakeConnection.query() remains unchanged
• Existing code that doesn't use params will continue to work identically
• The only behavioral change is that queries with different params values will now be cached separately (which is the correct/expected behavior)

Security & Risk

No security concerns identified.

Low regression risk:

• The change is minimal and targeted
• The fix aligns the implementation with the existing SQLConnection.query() pattern
• The test provides good coverage of the fix
• No changes to public API signatures

Recommendations

No blocking issues. The PR is well-implemented with appropriate test coverage.

Minor suggestions (non-blocking):

1. Consider adding a brief inline comment explaining why params must be an explicit parameter (to include it in the cache key), similar to the existing comment about __qualname__ manipulation. However, this is optional as the code is self-documenting given the context.

2. The test could optionally verify that the execute calls received the correct params values using mock_cursor.execute.assert_any_call(), but the current assertion is sufficient to verify the caching behavior.

Verdict

APPROVED: This is a correct and well-tested bug fix that addresses a real caching issue where different query parameters incorrectly shared cache entries. The implementation is consistent with existing patterns in SQLConnection.query() and poses no backwards compatibility or security risks.

This is an automated AI review using opus-4.5-thinking. Please verify the feedback and use your judgment.