fix(query-object): extra time-range-endpoints

[john-bodley]

SUMMARY

I'm really perplexed why this issue has recently surfaced and/or been reported and why it only applies to certain visualization types (is there any chance this is overridden by various chart types), but it seems like the Python date format wasn't being adhered to due to poorly formed extra time-range-endpoints.

More specifically for the QueryObject the extra fields are coming from superset-ui where, if defined, the time_range_endpoints is a tuple of strings which are never converted to a tuple of TimeRangeEndpoint enums and thus this check is false meaning the default date format is never adhered to.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[ktmud]

I don't really get why is time_range_endpoints a prerequisite for this date format conversion... The date format passed to database should not be incorrect even if time_range_endpoints are not set, right?

[ktmud]

There are recent updates to both both Time Picker module and the table chart (which migrated to the new API v1 endpoint) so a lot of things could go wrong in this process...

[villebro]

Oh wow, that would explain some of the problems people have been having with time range endpoints not working correctly. Passing self.extras as form_data feels slightly wrong; should we change the sig so that it's def get_time_range_endpoints(extras: ExtraFormData) or similar and perhaps add a TypedDict for it in superset/utils/core.py like here?

superset/superset/utils/core.py

Lines 175 to 177 in 8ab45c9

	class DatasourceDict(TypedDict):
	type: str
	id: int

[john-bodley]

@villebro the issue is that under other scenarios the full form data is required. We could have slots for both form_data and extras but I think that adds more confusion.

[ktmud]

I think there is a test case:

superset/tests/query_context_tests.py

Lines 170 to 185 in 0a00153

	def test_query_context_time_range_endpoints(self):
	"""
	Ensure that time_range_endpoints are populated automatically when missing
	from the payload.
	"""
	self.login(username="admin")
	payload = get_query_context("birth_names")
	del payload["queries"][0]["extras"]["time_range_endpoints"]
	query_context = ChartDataQueryContextSchema().load(payload)
	query_object = query_context.queries[0]
	extras = query_object.to_dict()["extras"]
	assert "time_range_endpoints" in extras
	self.assertEqual(
	extras["time_range_endpoints"],
	(TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE),
	)

that captures the intention of the original code.

Do you mind adding a test case so your change is also tested?

[john-bodley]

@ktmud this change captures the sentiment of the test, i.e., ensuring that the in extras the data is encoded as an enum rather than string, unless I'm confused regarding the ChartDataQueryContextSchema behavior.

[john-bodley]

@ktmud it's been a while since I wrote this code, but I believe the time_range_endpoints is a way of determining the state of a given chart when SIP-15 is enabled and the grace period hasn't completed, i.e., old charts could be using [start, end] whereas charts with [start, end) indicate the desired end state. It's mentioned here, though re-reading what I previously wrote it's not well worded.

[codecov]

Codecov Report

Merging #13331 (35bf533) into master (0a00153) will increase coverage by 3.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #13331      +/-   ##
==========================================
+ Coverage   77.19%   80.42%   +3.22%     
==========================================
  Files         872      300     -572     
  Lines       45101    24420   -20681     
  Branches     5435        0    -5435     
==========================================
- Hits        34817    19639   -15178     
+ Misses      10161     4781    -5380     
+ Partials      123        0     -123     

Flag	Coverage Δ
cypress	?
javascript	?
presto	80.01% <100.00%> (?)
python	80.42% <100.00%> (-0.42%)	⬇️
sqlite	79.96% <100.00%> (?)

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

Impacted Files	Coverage Δ
superset/charts/schemas.py	100.00% <100.00%> (ø)
superset/common/query_object.py	90.27% <100.00%> (ø)
superset/db_engines/hive.py	0.00% <0.00%> (-85.72%)	⬇️
superset/sql_validators/postgres.py	50.00% <0.00%> (-50.00%)	⬇️
superset/db_engine_specs/hive.py	74.23% <0.00%> (-16.54%)	⬇️
superset/databases/commands/create.py	83.67% <0.00%> (-8.17%)	⬇️
superset/databases/commands/update.py	85.71% <0.00%> (-8.17%)	⬇️
superset/reports/notifications/slack.py	83.72% <0.00%> (-5.47%)	⬇️
superset/databases/commands/test_connection.py	84.78% <0.00%> (-4.35%)	⬇️
superset/databases/api.py	89.51% <0.00%> (-2.42%)	⬇️
... and 584 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a00153...35bf533. Read the comment docs.

[john-bodley]

@ktmud and @villebro this is ready for re-review.

[ktmud]

LGTM

[ktmud]

I think there is a test case:

superset/tests/query_context_tests.py

Lines 170 to 185 in 0a00153

	def test_query_context_time_range_endpoints(self):
	"""
	Ensure that time_range_endpoints are populated automatically when missing
	from the payload.
	"""
	self.login(username="admin")
	payload = get_query_context("birth_names")
	del payload["queries"][0]["extras"]["time_range_endpoints"]
	query_context = ChartDataQueryContextSchema().load(payload)
	query_object = query_context.queries[0]
	extras = query_object.to_dict()["extras"]
	assert "time_range_endpoints" in extras
	self.assertEqual(
	extras["time_range_endpoints"],
	(TimeRangeEndpoint.INCLUSIVE, TimeRangeEndpoint.EXCLUSIVE),
	)

that captures the intention of the original code.

Do you mind adding a test case so your change is also tested?