Skip to content

fix(chart): allow null for optional query object props#12905

Merged
ktmud merged 1 commit intoapache:masterfrom
ktmud:null-query-object-props
Feb 3, 2021
Merged

fix(chart): allow null for optional query object props#12905
ktmud merged 1 commit intoapache:masterfrom
ktmud:null-query-object-props

Conversation

@ktmud
Copy link
Copy Markdown
Member

@ktmud ktmud commented Feb 3, 2021

SUMMARY

This is probably a little controversial, too.

Sometimes chart plugin buildQuery generates invalid null values for certain props (either because of sloppy code or uncleaned legacy data), making the whole query fails:

image

Since most of the props are optional anyway, maybe we should just make the backend more error-tolerant by allowing null (None) everywhere possible.

TEST PLAN

  • Added a unit test to check a query with granularity: null can still load and it has the same cache key as when granularity is not set.`

  • Manually tested by sending API request with null values to /api/v1/chart/data

{
	"datasource": {
		"id": 27,
		"type": "table"
	},
	"force": false,
	"queries": [
		{
			"time_range": null,
			"filters": null,
			"columns": [
				"season",
				"episode",
				"TITLE"
			],
			"metrics": null,
			"orderby": null,
			"row_limit": null,
			"timeseries_limit": null,
			"order_desc": null,
			"url_params": null,
			"post_processing": null
		}
	],
	"result_format": "json",
	"result_type": "full"
}

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 ktmud force-pushed the null-query-object-props branch from 6066edb to 521450d Compare February 3, 2021 01:16
@ktmud ktmud force-pushed the null-query-object-props branch from 521450d to 6c4a999 Compare February 3, 2021 01:43
@pull-request-size pull-request-size bot added size/M and removed size/S labels Feb 3, 2021
Copy link
Copy Markdown
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think is good. In general I try to follow Postel's Law: "Be conservative in what you send, be liberal in what you accept"

@ktmud
Copy link
Copy Markdown
Member Author

ktmud commented Feb 3, 2021

That's a good quote!

@codecov-io
Copy link
Copy Markdown

codecov-io commented Feb 3, 2021

Codecov Report

Merging #12905 (6c4a999) into master (5a79a7d) will decrease coverage by 0.42%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #12905      +/-   ##
==========================================
- Coverage   66.98%   66.56%   -0.43%     
==========================================
  Files        1026     1026              
  Lines       50330    50744     +414     
  Branches     5189     5361     +172     
==========================================
+ Hits        33713    33777      +64     
- Misses      16483    16832     +349     
- Partials      134      135       +1     
Flag Coverage Δ
cypress 50.82% <ø> (+0.01%) ⬆️
javascript 61.83% <ø> (ø)
python 63.95% <100.00%> (-0.17%) ⬇️

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-frontend/src/SqlLab/actions/sqlLab.js 44.81% <0.00%> (-19.09%) ⬇️
superset/db_engine_specs/presto.py 82.25% <0.00%> (-6.28%) ⬇️
superset/db_engine_specs/sqlite.py 90.62% <0.00%> (-6.25%) ⬇️
...et-frontend/src/SqlLab/reducers/getInitialState.js 46.42% <0.00%> (-3.58%) ⬇️
superset/utils/celery.py 86.20% <0.00%> (-3.45%) ⬇️
superset-frontend/src/SqlLab/reducers/sqlLab.js 37.59% <0.00%> (-2.49%) ⬇️
superset/db_engine_specs/mysql.py 89.79% <0.00%> (-2.05%) ⬇️
superset/result_set.py 96.69% <0.00%> (-1.66%) ⬇️
superset/models/core.py 87.77% <0.00%> (-1.09%) ⬇️
... and 8 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 5a79a7d...6c4a999. Read the comment docs.

@ktmud ktmud merged commit 9fa52d3 into apache:master Feb 3, 2021
@ktmud ktmud deleted the null-query-object-props branch February 3, 2021 03:28
amitmiran137 pushed a commit to nielsen-oss/superset that referenced this pull request Feb 3, 2021
* master: (23 commits)
  feat(explore): clear search on dataset change (apache#12909)
  chore: remove SIP-38 feature flag (apache#12894)
  fix: Config for dataset health check (apache#12906)
  fix(chart): allow null for most query object props (apache#12905)
  feat: add separate endpoint to fetch function names for autocomplete (apache#12840)
  chore: add required review on master (apache#12694)
  fix: comment typo (apache#12898)
  Migrates Radio component from Bootstrap to AntD. (apache#12738)
  fix: allow users to reset their passwords (apache#12886)
  fix(explore): missing select when groupby without metrics (apache#12890)
  refactor: dbapi exception mapping for dbapi's (apache#12869)
  feat(style-theme): add support for custom superset themes (apache#12858)
  chore(lint): fix pre-commit error (apache#12884)
  refactor(color-schemes): refactor setting of color schemes (apache#12857)
  feat(native-filters): Add defaultValue for Native filters modal (apache#12199)
  feat(release): add github token to changelog script (apache#12872)
  fix(menu): always show settings dropdown (apache#12877)
  Migrates Label component from Bootstrap to AntD. (apache#12774)
  [Helm] Automate datasource import (apache#10771)
  build: Skip loading example data from configs in CI (apache#12610)
  ...
villebro pushed a commit to preset-io/superset that referenced this pull request Feb 4, 2021
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.2.0 First shipped in 1.2.0 labels Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/M 🚢 1.2.0 First shipped in 1.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants