refactor(explore): extract session ID retrieval into overridable method by aminghadersohi · Pull Request #35779 · apache/superset

aminghadersohi · 2025-10-22T03:09:05Z

SUMMARY

This PR extracts the session ID retrieval logic in CreateFormDataCommand into a separate _get_session_id() method that can be overridden in subclasses. This improves testability and allows for custom session ID handling in derived classes.

Changes:

Extract session.get('_id') into _get_session_id() method in CreateFormDataCommand
Add docstring explaining the method can be overridden in subclasses
Update run() method to use self._get_session_id() instead of directly accessing session
Add comprehensive unit tests covering:
- Default behavior (returns session ID from Flask session)
- Subclass override capability
- Integration with run() method for cache key generation

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A - Backend refactoring with no UI changes

TESTING INSTRUCTIONS

Run the new unit tests:

pytest tests/unit_tests/commands/explore/form_data/test_create.py -v

Verify all 3 tests pass

Run existing integration tests to ensure no regression:

pytest tests/integration_tests/explore/form_data/commands_tests.py::TestCreateFormDataCommand -v

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

bito-code-review · 2025-10-22T03:09:14Z

Code Review Agent Run #f1f39c

Actionable Suggestions - 0

Additional Suggestions - 6

superset/commands/explore/form_data/create.py - 1

Missing trailing comma in function call · Line 54-54

Missing trailing comma in the `cache_key` function call. Add a trailing comma after the last parameter for consistency.

Code suggestion

 @@ -53,3 +53,3 @@
-            contextual_key = cache_key(
-                self._get_session_id(), tab_id, datasource_id, chart_id, datasource_type
-            )
+            contextual_key = cache_key(
+                self._get_session_id(), tab_id, datasource_id, chart_id, datasource_type,
+            )

tests/unit_tests/commands/explore/form_data/__init__.py - 1
- Missing docstring in public package · Line 1-1
  
  The package `__init__.py` file is missing a docstring. Consider adding a module-level docstring to document the package's purpose and contents.
  Code suggestion
  @@ -16,0 +17,3 @@ # under the License. +""" +Form data utilities for explore commands testing. +"""
tests/unit_tests/commands/explore/__init__.py - 1
- Missing docstring in public package · Line 1-1
  
  The package `__init__.py` file is missing a docstring. Consider adding a module-level docstring to document the package's purpose and contents.
  Code suggestion
  @@ -16,0 +17,3 @@ # under the License. +""" +Unit tests for explore commands. +"""
tests/unit_tests/commands/explore/form_data/test_create.py - 3
- Missing module docstring documentation · Line 1-1
  
  Module is missing a docstring. Consider adding a brief description of the module's purpose at the top of the file.
  Code suggestion
  @@ -17,6 +17,9 @@ -from unittest.mock import MagicMock, patch +"""Tests for CreateFormDataCommand functionality.""" + +from unittest.mock import MagicMock, patch
- Missing return type annotation for function · Line 24-24
  
  Test functions are missing return type annotations. Consider adding `-> None` to all three test functions (`test_get_session_id_returns_session_id`, `test_get_session_id_can_be_overridden`, `test_run_uses_get_session_id`).
  Code suggestion
  @@ -24,6 +24,6 @@ -def test_get_session_id_returns_session_id(): +def test_get_session_id_returns_session_id() -> None:
- Missing trailing commas in multiline contexts · Line 82-82
  
  Multiple trailing commas are missing in the `patch` context manager calls (lines 82, 85, 88, 91, 105). Consider adding trailing commas for consistency.
  Code suggestion
  @@ -80,6 +80,6 @@ - patch("superset.commands.explore.form_data.create.cache_key") as mock_cache_key, + patch("superset.commands.explore.form_data.create.cache_key") as mock_cache_key,

Review Details

Files reviewed - 4 · Commit Range: b2ea463..b2ea463
- superset/commands/explore/form_data/create.py
- tests/unit_tests/commands/explore/__init__.py
- tests/unit_tests/commands/explore/form_data/__init__.py
- tests/unit_tests/commands/explore/form_data/test_create.py
Files skipped - 0
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.
/pause - Pauses automatic reviews on this pull request.
/resume - Resumes automatic reviews.
/resolve - Marks all Bito-posted review comments as resolved.
/abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by

korbit-ai

I've completed my review and didn't find any issues.

Files scanned

File Path	Reviewed
superset/commands/explore/form_data/create.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

bito-code-review · 2025-10-22T03:14:07Z

Interaction Diagram by Bito

sequenceDiagram
participant API as ExploreFormDataRestApi<br/>🔄 Updated | ●●○ Medium
participant CMD as CreateFormDataCommand<br/>🔄 Updated | ●●○ Medium
participant SES as Flask Session
participant ACC as Access Control
participant CACHE as Cache Manager
participant KEY as Cache Key Utils<br/>🔄 Updated | ●●○ Medium
Note over CMD: Added _get_session_id()<br/>method for testability
API->>CMD: CreateFormDataCommand(params).run()
CMD->>CMD: _get_session_id()
CMD->>SES: session.get("_id")
SES-->>CMD: session_id
CMD->>ACC: check_access(datasource_id, chart_id, type)
ACC-->>CMD: access_granted
CMD->>KEY: cache_key(session_id, tab_id, datasource_id, chart_id, type)
KEY-->>CMD: contextual_key
CMD->>CACHE: explore_form_data_cache.get/set
CACHE-->>API: form_data_key

Critical path: ExploreFormDataRestApi->CreateFormDataCommand->Flask Session->Cache Key Utils->Cache Manager

Note: The CreateFormDataCommand refactored session ID retrieval into a separate _get_session_id() method, replacing direct session.get() calls. This improves testability by allowing subclasses to override session behavior while maintaining the same upstream API and downstream cache operations.

codecov · 2025-10-22T03:24:14Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.03%. Comparing base (e145505) to head (86bf532).
⚠️ Report is 14 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #35779       +/-   ##
===========================================
+ Coverage        0   71.03%   +71.03%     
===========================================
  Files           0      605      +605     
  Lines           0    44209    +44209     
  Branches        0     4782     +4782     
===========================================
+ Hits            0    31406    +31406     
- Misses          0    11558    +11558     
- Partials        0     1245     +1245

Flag	Coverage Δ
hive	`45.70% <50.00%> (?)`
mysql	`70.08% <100.00%> (?)`
postgres	`70.13% <100.00%> (?)`
presto	`49.35% <50.00%> (?)`
python	`71.00% <100.00%> (?)`
sqlite	`69.73% <100.00%> (?)`
unit	`100.00% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

sadpandajoe · 2025-10-22T17:59:35Z

    def __init__(self, cmd_params: CommandParameters):
        self._cmd_params = cmd_params

+    def _get_session_id(self) -> str:


can't this function also return None? as session.get("_id") could not exist, right?

sadpandajoe · 2025-10-22T18:08:52Z

+from superset.utils.core import DatasourceType
+
+
+def test_get_session_id_returns_session_id():


Do we still need this test if test_run_uses_get_session_id already calls _get_session_id?

you are right i will get rid of it

- Extract session.get('_id') into _get_session_id() method in CreateFormDataCommand - Allow subclasses to override session ID retrieval for custom behavior - Add comprehensive unit tests for the new method

sadpandajoe

LGTM

…od (#35779) (cherry picked from commit 6f50ddf)

…od (apache#35779) (cherry picked from commit 6f50ddf)

…od (apache#35779)

pull-request-size Bot added the size/L label Oct 22, 2025

dosubot Bot added the explore:refactor Related to refactoring Explore label Oct 22, 2025

korbit-ai Bot approved these changes Oct 22, 2025

View reviewed changes

sadpandajoe requested review from michael-s-molina and sadpandajoe October 22, 2025 17:22

sadpandajoe reviewed Oct 22, 2025

View reviewed changes

aminghadersohi added 2 commits October 28, 2025 18:34

refactor(explore): extract session ID retrieval into overridable method

8b3c0d4

- Extract session.get('_id') into _get_session_id() method in CreateFormDataCommand - Allow subclasses to override session ID retrieval for custom behavior - Add comprehensive unit tests for the new method

update

86bf532

aminghadersohi force-pushed the chore/command_session_override branch from b2ea463 to 86bf532 Compare October 28, 2025 22:36

sadpandajoe approved these changes Oct 29, 2025

View reviewed changes

sadpandajoe merged commit 6f50ddf into apache:master Oct 30, 2025
64 checks passed

sadpandajoe added 6.0:checkpoint and removed 6.0:checkpoint labels Oct 30, 2025

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2025

refactor(explore): extract session ID retrieval into overridable meth…

264e0d5

…od (#35779) (cherry picked from commit 6f50ddf)

sadpandajoe pushed a commit that referenced this pull request Dec 4, 2025

refactor(explore): extract session ID retrieval into overridable meth…

728942e

…od (#35779) (cherry picked from commit 6f50ddf)

sadpandajoe pushed a commit that referenced this pull request Dec 16, 2025

refactor(explore): extract session ID retrieval into overridable meth…

2e224de

…od (#35779) (cherry picked from commit 6f50ddf)

aminghadersohi added a commit to aminghadersohi/superset that referenced this pull request Jan 17, 2026

refactor(explore): extract session ID retrieval into overridable meth…

f1b6c69

…od (apache#35779) (cherry picked from commit 6f50ddf)

aminghadersohi added a commit to aminghadersohi/superset that referenced this pull request Jan 24, 2026

refactor(explore): extract session ID retrieval into overridable meth…

c49a5de

…od (apache#35779) (cherry picked from commit 6f50ddf)

qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026

refactor(explore): extract session ID retrieval into overridable meth…

2182792

…od (apache#35779)

Navigation Menu

Uh oh!

refactor(explore): extract session ID retrieval into overridable method - #35779

refactor(explore): extract session ID retrieval into overridable method#35779
sadpandajoe merged 2 commits into
apache:masterfrom
aminghadersohi:chore/command_session_override

aminghadersohi commented Oct 22, 2025

Uh oh!

bito-code-review Bot commented Oct 22, 2025 •

edited

Loading

Code Review Agent Run #f1f39c

Uh oh!

korbit-ai Bot left a comment

Uh oh!

bito-code-review Bot commented Oct 22, 2025

Uh oh!

codecov Bot commented Oct 22, 2025 •

edited

Loading

Uh oh!

sadpandajoe Oct 22, 2025

Uh oh!

aminghadersohi Oct 28, 2025

Uh oh!

aminghadersohi Oct 28, 2025

Uh oh!

sadpandajoe Oct 22, 2025

Uh oh!

aminghadersohi Oct 28, 2025

Uh oh!

aminghadersohi Oct 28, 2025

Uh oh!

sadpandajoe left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		from superset.utils.core import DatasourceType


		def test_get_session_id_returns_session_id():

Uh oh!

Conversation

aminghadersohi commented Oct 22, 2025

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #f1f39c

Uh oh!

korbit-ai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Oct 22, 2025

Uh oh!

codecov Bot commented Oct 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sadpandajoe Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

aminghadersohi Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

aminghadersohi Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

aminghadersohi Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

aminghadersohi Oct 28, 2025

Choose a reason for hiding this comment

Uh oh!

sadpandajoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bito-code-review Bot commented Oct 22, 2025 •

edited

Loading

codecov Bot commented Oct 22, 2025 •

edited

Loading