feat: Normalize orderby clause (#1206)

* feat: normalize orderby clause

* fix ut

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/query/index.ts

@@ -26,6 +26,7 @@ export { default as convertFilter } from './convertFilter';
 export { default as extractTimegrain } from './extractTimegrain';
 export { default as getMetricLabel } from './getMetricLabel';
 export { default as DatasourceKey } from './DatasourceKey';
+export { default as normalizeOrderBy } from './normalizeOrderBy';
 
 export * from './types/AnnotationLayer';
 export * from './types/QueryFormData';

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/src/query/normalizeOrderBy.ts

@@ -0,0 +1,64 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import isEmpty from 'lodash/isEmpty';
+import isBoolean from 'lodash/isBoolean';
+
+import { QueryObject } from './types';
+
+export default function normalizeOrderBy(queryObject: QueryObject): QueryObject {
+  if (Array.isArray(queryObject.orderby) && queryObject.orderby.length > 0) {
+    // ensure a valid orderby clause
+    const orderbyClause = queryObject.orderby[0];
+    if (
+      Array.isArray(orderbyClause) &&
+      orderbyClause.length === 2 &&
+      !isEmpty(orderbyClause[0]) &&
+      isBoolean(orderbyClause[1])
+    ) {
+      return queryObject;
+    }
+  }
+
+  // ensure that remove invalid orderby clause
+  const cloneQueryObject = { ...queryObject };
+  delete cloneQueryObject.timeseries_limit_metric;
+  delete cloneQueryObject.order_desc;
+  delete cloneQueryObject.orderby;
+
+  const isAsc = !queryObject.order_desc;
+  if (
+    queryObject.timeseries_limit_metric !== undefined &&
+    queryObject.timeseries_limit_metric !== null &&
+    !isEmpty(queryObject.timeseries_limit_metric)
+  ) {
+    return {
+      ...cloneQueryObject,
+      orderby: [[queryObject.timeseries_limit_metric, isAsc]],
+    };
+  }
+
+  if (Array.isArray(queryObject.metrics) && queryObject.metrics.length > 0) {
+    return {
+      ...cloneQueryObject,
+      orderby: [[queryObject.metrics[0], isAsc]],
+    };
+  }
+
+  return cloneQueryObject;
+}

superset-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-core/test/query/normalizeOrderBy.test.ts

@@ -0,0 +1,150 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { normalizeOrderBy, QueryObject } from '@superset-ui/core/src/query';
+
+describe('normalizeOrderBy', () => {
+  it('should not change original queryObject when orderby populated', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [['count(*)', true]],
+    };
+    expect(normalizeOrderBy(query)).toEqual(query);
+  });
+
+  it('has timeseries_limit_metric in queryObject', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      metrics: ['count(*)'],
+      timeseries_limit_metric: {
+        expressionType: 'SIMPLE',
+        column: {
+          id: 1,
+          column_name: 'sales',
+        },
+        aggregate: 'SUM',
+      },
+      order_desc: true,
+    };
+    const expectedQueryObject = normalizeOrderBy(query);
+    expect(expectedQueryObject).not.toHaveProperty('timeseries_limit_metric');
+    expect(expectedQueryObject).not.toHaveProperty('order_desc');
+    expect(expectedQueryObject).toEqual({
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      metrics: ['count(*)'],
+      orderby: [
+        [
+          {
+            expressionType: 'SIMPLE',
+            column: {
+              id: 1,
+              column_name: 'sales',
+            },
+            aggregate: 'SUM',
+          },
+          false,
+        ],
+      ],
+    });
+  });
+
+  it('has metrics in queryObject', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      metrics: ['count(*)'],
+      order_desc: true,
+    };
+    const expectedQueryObject = normalizeOrderBy(query);
+    expect(expectedQueryObject).not.toHaveProperty('timeseries_limit_metric');
+    expect(expectedQueryObject).not.toHaveProperty('order_desc');
+    expect(expectedQueryObject).toEqual({
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      metrics: ['count(*)'],
+      orderby: [['count(*)', false]],
+    });
+  });
+
+  it('should not change', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+    };
+    expect(normalizeOrderBy(query)).toEqual(query);
+  });
+
+  it('remove empty orderby', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [],
+    };
+    expect(normalizeOrderBy(query)).not.toHaveProperty('orderby');
+  });
+
+  it('remove orderby with an empty array', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [[]],
+    };
+    expect(normalizeOrderBy(query)).not.toHaveProperty('orderby');
+  });
+
+  it('remove orderby with an empty metric', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [['', true]],
+    };
+    expect(normalizeOrderBy(query)).not.toHaveProperty('orderby');
+  });
+
+  it('remove orderby with an empty adhoc metric', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [[{}, true]],
+    };
+    expect(normalizeOrderBy(query)).not.toHaveProperty('orderby');
+  });
+
+  it('remove orderby with an non-boolean type', () => {
+    const query: QueryObject = {
+      datasource: '5__table',
+      viz_type: 'table',
+      time_range: '1 year ago : 2013',
+      orderby: [['count(*)', 'true']],
+    };
+    expect(normalizeOrderBy(query)).not.toHaveProperty('orderby');
+  });
+});

superset-frontend/temporary_superset_ui/superset-ui/plugins/plugin-chart-echarts/src/MixedTimeseries/buildQuery.ts

@@ -16,7 +16,13 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { buildQueryContext, getMetricLabel, QueryFormData } from '@superset-ui/core';
+import {
+  buildQueryContext,
+  getMetricLabel,
+  QueryFormData,
+  QueryObject,
+  normalizeOrderBy,
+} from '@superset-ui/core';
 
 export default function buildQuery(formData: QueryFormData) {
   const {
@@ -56,49 +62,46 @@ export default function buildQuery(formData: QueryFormData) {
 
   const queryContextA = buildQueryContext(formData1, baseQueryObject => {
     const metricLabels = (baseQueryObject.metrics || []).map(getMetricLabel);
-    return [
-      {
-        ...baseQueryObject,
-        is_timeseries: true,
-        orderby: timeseries_limit_metric ? [[timeseries_limit_metric, !order_desc]] : [],
-        post_processing: [
-          {
-            operation: 'pivot',
-            options: {
-              index: ['__timestamp'],
-              columns: formData1.columns || [],
-              // Create 'dummy' sum aggregates to assign cell values in pivot table
-              aggregates: Object.fromEntries(
-                metricLabels.map(metric => [metric, { operator: 'sum' }]),
-              ),
-            },
+    const queryObjectA = {
+      ...baseQueryObject,
+      is_timeseries: true,
+      post_processing: [
+        {
+          operation: 'pivot',
+          options: {
+            index: ['__timestamp'],
+            columns: formData1.columns || [],
+            // Create 'dummy' sum aggregates to assign cell values in pivot table
+            aggregates: Object.fromEntries(
+              metricLabels.map(metric => [metric, { operator: 'sum' }]),
+            ),
           },
-        ],
-      },
-    ];
+        },
+      ],
+    } as QueryObject;
+    return [normalizeOrderBy(queryObjectA)];
   });
+
   const queryContextB = buildQueryContext(formData2, baseQueryObject => {
     const metricLabels = (baseQueryObject.metrics || []).map(getMetricLabel);
-    return [
-      {
-        ...baseQueryObject,
-        is_timeseries: true,
-        orderby: timeseries_limit_metric_b ? [[timeseries_limit_metric_b, !order_desc_b]] : [],
-        post_processing: [
-          {
-            operation: 'pivot',
-            options: {
-              index: ['__timestamp'],
-              columns: formData2.columns || [],
-              // Create 'dummy' sum aggregates to assign cell values in pivot table
-              aggregates: Object.fromEntries(
-                metricLabels.map(metric => [metric, { operator: 'sum' }]),
-              ),
-            },
+    const queryObjectB = {
+      ...baseQueryObject,
+      is_timeseries: true,
+      post_processing: [
+        {
+          operation: 'pivot',
+          options: {
+            index: ['__timestamp'],
+            columns: formData2.columns || [],
+            // Create 'dummy' sum aggregates to assign cell values in pivot table
+            aggregates: Object.fromEntries(
+              metricLabels.map(metric => [metric, { operator: 'sum' }]),
+            ),
           },
-        ],
-      },
-    ];
+        },
+      ],
+    } as QueryObject;
+    return [normalizeOrderBy(queryObjectB)];
   });
 
   return {