From 4c0e0b479309d21330c3d8b21a015d6e21667546 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Wed, 3 Feb 2021 20:26:28 -1000 Subject: [PATCH 1/2] fix(plugin-chart-table): always sort descending by first metric --- .../src/query/buildQueryObject.ts | 16 ++++--- .../src/utils/ensureIsArray.ts | 16 +++---- packages/superset-ui-core/src/utils/index.ts | 1 + .../test/utils/ensureIsArray.test.ts | 21 ++++----- plugins/plugin-chart-table/src/buildQuery.ts | 45 +++++++++++++------ .../plugin-chart-table/src/controlPanel.tsx | 5 ++- 6 files changed, 61 insertions(+), 43 deletions(-) rename plugins/plugin-chart-table/src/utils/extractOrderby.ts => packages/superset-ui-core/src/utils/ensureIsArray.ts (68%) rename plugins/plugin-chart-table/test/utils.test.ts => packages/superset-ui-core/test/utils/ensureIsArray.test.ts (56%) diff --git a/packages/superset-ui-core/src/query/buildQueryObject.ts b/packages/superset-ui-core/src/query/buildQueryObject.ts index fb556e581e..5f297ce274 100644 --- a/packages/superset-ui-core/src/query/buildQueryObject.ts +++ b/packages/superset-ui-core/src/query/buildQueryObject.ts @@ -47,22 +47,24 @@ export default function buildQueryObject( }); let queryObject: QueryObject = { - time_range, - since, - until, - granularity, + // fallback `null` to `undefined` so they won't be sent to the backend + // (JSON.strinify will ignore `undefined`.) + time_range: time_range || undefined, + since: since || undefined, + until: until || undefined, + granularity: granularity || undefined, ...extras, ...extrasAndfilters, - annotation_layers, columns, metrics, orderby, + annotation_layers, row_limit: row_limit == null || Number.isNaN(numericRowLimit) ? undefined : numericRowLimit, row_offset: row_offset == null || Number.isNaN(numericRowOffset) ? undefined : numericRowOffset, timeseries_limit: limit ? Number(limit) : 0, - timeseries_limit_metric, + timeseries_limit_metric: timeseries_limit_metric || undefined, order_desc: typeof order_desc === 'undefined' ? true : order_desc, - url_params, + url_params: url_params || undefined, }; // append and override extra form data used by native filters queryObject = appendExtraFormData(queryObject, append_form_data); diff --git a/plugins/plugin-chart-table/src/utils/extractOrderby.ts b/packages/superset-ui-core/src/utils/ensureIsArray.ts similarity index 68% rename from plugins/plugin-chart-table/src/utils/extractOrderby.ts rename to packages/superset-ui-core/src/utils/ensureIsArray.ts index d7a5007f96..244643f1df 100644 --- a/plugins/plugin-chart-table/src/utils/extractOrderby.ts +++ b/packages/superset-ui-core/src/utils/ensureIsArray.ts @@ -16,16 +16,14 @@ * specific language governing permissions and limitations * under the License. */ -import { QueryFormMetric } from '@superset-ui/core'; -export function extractTimeseriesLimitMetric( - timeSeriesLimitMetric?: QueryFormMetric[] | QueryFormMetric | null, -): QueryFormMetric[] { - if (timeSeriesLimitMetric === undefined || timeSeriesLimitMetric === null) { +/** + * Ensure a nullable value input is an array. Useful when consolidating + * input format from a select control. + */ +export default function ensureIsArray(value?: T[] | T | null): T[] { + if (value === undefined || value === null) { return []; } - if (Array.isArray(timeSeriesLimitMetric)) { - return timeSeriesLimitMetric; - } - return [timeSeriesLimitMetric]; + return Array.isArray(value) ? value : [value]; } diff --git a/packages/superset-ui-core/src/utils/index.ts b/packages/superset-ui-core/src/utils/index.ts index a79dd297d8..df0ea96e16 100644 --- a/packages/superset-ui-core/src/utils/index.ts +++ b/packages/superset-ui-core/src/utils/index.ts @@ -17,6 +17,7 @@ * under the License. */ export { default as convertKeysToCamelCase } from './convertKeysToCamelCase'; +export { default as ensureIsArray } from './ensureIsArray'; export { default as isDefined } from './isDefined'; export { default as isRequired } from './isRequired'; export { default as makeSingleton } from './makeSingleton'; diff --git a/plugins/plugin-chart-table/test/utils.test.ts b/packages/superset-ui-core/test/utils/ensureIsArray.test.ts similarity index 56% rename from plugins/plugin-chart-table/test/utils.test.ts rename to packages/superset-ui-core/test/utils/ensureIsArray.test.ts index 772cb1f117..022e970dab 100644 --- a/plugins/plugin-chart-table/test/utils.test.ts +++ b/packages/superset-ui-core/test/utils/ensureIsArray.test.ts @@ -16,18 +16,15 @@ * specific language governing permissions and limitations * under the License. */ -import { extractTimeseriesLimitMetric } from '../src/utils/extractOrderby'; +import { ensureIsArray } from '../../src'; -describe('utils', () => { - it('should add post-processing in aggregate mode', () => { - expect(extractTimeseriesLimitMetric(undefined)).toEqual([]); - expect(extractTimeseriesLimitMetric(null)).toEqual([]); - expect(extractTimeseriesLimitMetric([])).toEqual([]); - expect(extractTimeseriesLimitMetric('my_metric')).toEqual(['my_metric']); - expect(extractTimeseriesLimitMetric(['my_metric'])).toEqual(['my_metric']); - expect(extractTimeseriesLimitMetric(['my_metric_1', 'my_metric_2'])).toEqual([ - 'my_metric_1', - 'my_metric_2', - ]); +describe('ensureIsArray', () => { + it('handle inputs correctly', () => { + expect(ensureIsArray(undefined)).toEqual([]); + expect(ensureIsArray(null)).toEqual([]); + expect(ensureIsArray([])).toEqual([]); + expect(ensureIsArray('my_metric')).toEqual(['my_metric']); + expect(ensureIsArray(['my_metric'])).toEqual(['my_metric']); + expect(ensureIsArray(['my_metric_1', 'my_metric_2'])).toEqual(['my_metric_1', 'my_metric_2']); }); }); diff --git a/plugins/plugin-chart-table/src/buildQuery.ts b/plugins/plugin-chart-table/src/buildQuery.ts index 00aaf33714..4679080f4d 100644 --- a/plugins/plugin-chart-table/src/buildQuery.ts +++ b/plugins/plugin-chart-table/src/buildQuery.ts @@ -16,29 +16,48 @@ * specific language governing permissions and limitations * under the License. */ -import { buildQueryContext, getMetricLabel, QueryMode, removeDuplicates } from '@superset-ui/core'; +import { + buildQueryContext, + getMetricLabel, + QueryMode, + removeDuplicates, + ensureIsArray, +} from '@superset-ui/core'; import { PostProcessingRule } from '@superset-ui/core/src/query/types/PostProcessing'; import { TableChartFormData } from './types'; -import { extractTimeseriesLimitMetric } from './utils/extractOrderby'; + +/** + * Infer query mode from form data. If `all_columns` is set, then raw records mode, + * otherwise defaults to aggregation mode. + * + * The same logic is used in `controlPanel` with control values as well. + */ +export function getQueryMode(formData: TableChartFormData) { + const { query_mode: mode } = formData; + if (mode === QueryMode.aggregate || mode === QueryMode.raw) { + return mode; + } + const rawColumns = formData?.all_columns; + const hasRawColumns = rawColumns && rawColumns.length > 0; + return hasRawColumns ? QueryMode.raw : QueryMode.aggregate; +} export default function buildQuery(formData: TableChartFormData) { - const { percent_metrics: percentMetrics, order_desc: orderDesc = null } = formData; - let { query_mode: queryMode } = formData; - const timeseriesLimitMetric = extractTimeseriesLimitMetric(formData.timeseries_limit_metric); + const { percent_metrics: percentMetrics, order_desc: orderDesc = false } = formData; + const queryMode = getQueryMode(formData); + const sortByMetric = ensureIsArray(formData.timeseries_limit_metric)[0]; return buildQueryContext(formData, baseQueryObject => { let { metrics, orderby } = baseQueryObject; - if (queryMode === undefined && metrics.length > 0) { - queryMode = QueryMode.aggregate; - } let postProcessing: PostProcessingRule[] = []; if (queryMode === QueryMode.aggregate) { // orverride orderby with timeseries metric when in aggregation mode - if (timeseriesLimitMetric.length > 0 && orderDesc != null) { - orderby = [[timeseriesLimitMetric[0], !orderDesc]]; - } else if (timeseriesLimitMetric.length === 0 && metrics?.length > 0 && orderDesc != null) { - // default to ordering by first metric when no sort order has been specified - orderby = [[metrics[0], !orderDesc]]; + if (sortByMetric) { + orderby = [[sortByMetric, !orderDesc]]; + } else if (metrics?.length > 0) { + // default to ordering by first metric in descending order + // when no "sort by" metric is set (regargless if "SORT DESC" is set to true) + orderby = [[metrics[0], false]]; } // add postprocessing for percent metrics only when in aggregation mode if (percentMetrics && percentMetrics.length > 0) { diff --git a/plugins/plugin-chart-table/src/controlPanel.tsx b/plugins/plugin-chart-table/src/controlPanel.tsx index 0bf152a56a..ba22c45bc6 100644 --- a/plugins/plugin-chart-table/src/controlPanel.tsx +++ b/plugins/plugin-chart-table/src/controlPanel.tsx @@ -24,6 +24,7 @@ import { addLocaleData, smartDateFormatter, QueryMode, + QueryFormColumn, } from '@superset-ui/core'; import { formatSelectOptions, @@ -60,8 +61,8 @@ function getQueryMode(controls: ControlStateMapping): QueryMode { if (mode === QueryMode.aggregate || mode === QueryMode.raw) { return mode as QueryMode; } - const rawColumns = controls?.all_columns?.value; - const hasRawColumns = rawColumns && (rawColumns as string[])?.length > 0; + const rawColumns: QueryFormColumn[] | undefined = controls?.all_columns?.value; + const hasRawColumns = rawColumns && rawColumns.length > 0; return hasRawColumns ? QueryMode.raw : QueryMode.aggregate; } From 265135f900d11d0557a4075057323cefecde2820 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Wed, 3 Feb 2021 21:27:58 -1000 Subject: [PATCH 2/2] fix test coverage --- .../test/query/buildQueryObject.test.ts | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/superset-ui-core/test/query/buildQueryObject.test.ts b/packages/superset-ui-core/test/query/buildQueryObject.test.ts index c051cdfd3b..1fb6add78d 100644 --- a/packages/superset-ui-core/test/query/buildQueryObject.test.ts +++ b/packages/superset-ui-core/test/query/buildQueryObject.test.ts @@ -201,13 +201,21 @@ describe('buildQueryObject', () => { }); it('should populate url_params', () => { - const urlParams = { abc: '123' }; - query = buildQueryObject({ - datasource: '5__table', - granularity_sqla: 'ds', - viz_type: 'table', - url_params: urlParams, - }); - expect(query.url_params).toEqual(urlParams); + expect( + buildQueryObject({ + datasource: '5__table', + granularity_sqla: 'ds', + viz_type: 'table', + url_params: { abc: '123' }, + }).url_params, + ).toEqual({ abc: '123' }); + expect( + buildQueryObject({ + datasource: '5__table', + granularity_sqla: 'ds', + viz_type: 'table', + url_params: (null as unknown) as undefined, + }).url_params, + ).toBeUndefined(); }); });