refactor(QueryObject): add QueryObjectFactory to meet SRP #17466
+88
−52
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zhaoyongjie
reviewed
Nov 18, 2021
superset/common/query_object.py
Outdated
Comment on lines
79
to
102
| class QueryObjectFactory: | ||
| def create(self, | ||
| result_type: ChartDataResultType, | ||
| datasource: Optional[DatasourceDict] = None, | ||
| extras: Optional[Dict[str, Any]] = None, | ||
| row_limit: Optional[int] = None, | ||
| time_range: Optional[str] = None, | ||
| time_shift: Optional[str] = None, | ||
| **kwargs) -> QueryObject: | ||
| datasource_model_instance = None | ||
| if datasource: | ||
| datasource_model_instance = self._convert_to_model(datasource) | ||
| extras = self._process_extras(extras) | ||
| row_limit = self._process_row_limit(row_limit, result_type) | ||
| from_dttm, to_dttm = self._get_dttms(time_range, time_shift, extras) | ||
| return QueryObject(result_type, | ||
| datasource=datasource_model_instance, | ||
| extras=extras, | ||
| row_limit=row_limit, | ||
| from_dttm=from_dttm, | ||
| to_dttm=to_dttm, | ||
| time_range=time_range, | ||
| time_shift=time_shift, | ||
| **kwargs) |
There was a problem hiding this comment.
@classmethod
def create(cls,
result_type: ChartDataResultType,
datasource: Optional[DatasourceDict] = None,
extras: Optional[Dict[str, Any]] = None,
row_limit: Optional[int] = None,
time_range: Optional[str] = None,
time_shift: Optional[str] = None,
**kwargs) -> QueryObject:
query_object_instance = cls()
datasource_model_instance = None
if datasource:
datasource_model_instance = query_object_instance._convert_to_model(datasource)
extras = query_object_instance._process_extras(extras)
row_limit = query_object_instance._process_row_limit(row_limit, result_type)
from_dttm, to_dttm = query_object_instance._get_dttms(time_range, time_shift, extras)
return QueryObject(result_type,
datasource=datasource_model_instance,
extras=extras,
row_limit=row_limit,
from_dttm=from_dttm,
to_dttm=to_dttm,
time_range=time_range,
time_shift=time_shift,
**kwargs)
superset/common/query_context.py
Outdated
Comment on lines
107
to
108
| query_object_factory = QueryObjectFactory() | ||
| self.queries = [query_object_factory.create(**query_obj) for query_obj in queries] |
There was a problem hiding this comment.
self.queries = [QueryObjectFactory.create(**query_obj) for query_obj in queries]There was a problem hiding this comment.
- In general, using class methods should be avoided, hard to test.
- this is only a temp change, after that PR will be another PRs so the Factory will move to another module and config will be injected to it so it wont be couple to superset
There was a problem hiding this comment.
@zhaoyongjie look at #17479
There was a problem hiding this comment.
Thanks! I will take a closer look tomorrow.
ofekisr
force-pushed
the
refactor/chart_data_flows8
branch
from
e0a09b8
to
8dd6248
Compare
ofekisr
changed the title
Codecov Report
@@ Coverage Diff @@
## master #17466 +/- ##
==========================================
+ Coverage 76.87% 76.95% +0.08%
==========================================
Files 1042 1042
Lines 56331 56345 +14
Branches 7793 7793
==========================================
+ Hits 43304 43360 +56
+ Misses 12771 12729 -42
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This was referenced Nov 21, 2021
AAfghahi
pushed a commit
that referenced
this pull request
Jan 10, 2022
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
When we have worked on #16991 we wanted to test the new functionalities in concrete and accurate unittest.
All chartData flows and its components are too couple to superset so it is impossible to create unittests.
The flows are not testable and so many components do not meet the very important principle SRP and the code became so dirty
So I've started to refactor it (#17344 ) but many changes were added and it was hard to review so I decided to split those changes into small PRs so will be easier to follow
This is the eighth PR in a sequence of PRs to meet these
The next PR is #17479
PR description
Creating QueryObject is not only an assignment task, it contains some logic.
To meet the SRP, The creation task should be assigned to an external object.
After the PR the QueryObject constructor is decoupled from Superset so in the next PR, the module can be decoupled from Superset
Test plans
There is no logic added so new tests are not required
Previous PRs