mistercrunch opened a new pull request, #34177:
URL: https://github.com/apache/superset/pull/34177
Remove duplicate code between SqlaTable and ExploreMixin classes
This commit eliminates 7 duplicate methods totaling ~146 lines of code
that were
introduced by PR #20281 in July 2022. The duplication occurred when
@hughhhh
created the `ExploreMixin` class to enable Query objects to be visualized
in Explore,
but instead of properly refactoring shared functionality, he copy-pasted
methods
from SqlaTable into the new mixin.
### Root Cause Analysis
The code duplication was traced to commit
e5e886739460c011a885a13b873665410045a19c
where ExploreMixin was created with methods copied from SqlaTable:
- 5 methods were exact duplicates with identical implementations
- 2 methods had diverged over time with different error handling and
security practices
### Method Resolution Order Impact
SqlaTable inherits from (Model, BaseDatasource, ExploreMixin), meaning:
- SqlaTable's implementations override ExploreMixin's versions
- Query class (inheriting only from ExploreMixin) uses the ExploreMixin
versions
- Both sets of methods were being used, creating genuine duplication
### Methods Removed from SqlaTable
**Exact duplicates:**
- `filter_values_handler` (60 lines) - Static method for handling filter
values
- `_apply_cte` (12 lines) - Static method for appending CTEs to SQL
- `make_orderby_compatible` (28 lines) - Instance method for ORDER BY
compatibility
- `text` (2 lines) - Instance method for creating TextClause objects
- `get_query_str` (4 lines) - Instance method for generating query strings
**Divergent methods consolidated:**
- `_normalize_prequery_result_type` (35 lines) - Improved to use
ExploreMixin's
more robust version with better error handling for missing columns
- `changed_by_name` (5 lines) - Improved to use AuditMixinNullable's more
secure
version with HTML escaping instead of plain string conversion
### Technical Improvements
Beyond eliminating duplication, this refactoring improves the codebase by:
- Using safer `.get()` method instead of direct dict access for column
lookups
- Implementing proper HTML escaping for user-facing data (security
improvement)
- Maintaining single source of truth for shared functionality between
SqlaTable and Query
### Testing
All existing unit tests pass (8/8) and critical integration tests for the
normalize_prequery_result_type functionality have been verified. The
inheritance
hierarchy ensures SqlaTable now properly inherits the more robust
implementations
from ExploreMixin and AuditMixinNullable.
Fixes: Code duplication introduced in PR #20281 (feat: Visualize
SqlLab.Query model data in Explore)
--
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]
