From 82235b9e3d1a6dc0fcc59c6e240ba83bd06d54f0 Mon Sep 17 00:00:00 2001
From: Matthias Wilhelm <matthias.wilhelm@elastic.co>
Date: Tue, 20 Apr 2021 20:29:47 +0200
Subject: [PATCH] [Discover] Fix wrong sort order with empty sort URL parameter
 (#97434)

Co-authored-by: Tim Roes <mail@timroes.de>
# Conflicts:
#	src/plugins/discover/public/application/angular/discover_state.ts
---
 .../angular/discover_state.test.ts            | 42 +++++++++++++++++++
 .../application/angular/discover_state.ts     |  9 +++-
 .../functional/apps/discover/_shared_links.ts | 27 ++++++++++++
 3 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/src/plugins/discover/public/application/angular/discover_state.test.ts b/src/plugins/discover/public/application/angular/discover_state.test.ts
index e7322a8588631..ddb4e874ccc64 100644
--- a/src/plugins/discover/public/application/angular/discover_state.test.ts
+++ b/src/plugins/discover/public/application/angular/discover_state.test.ts
@@ -79,6 +79,48 @@ describe('Test discover state', () => {
     expect(state.getPreviousAppState()).toEqual(stateA);
   });
 });
+describe('Test discover initial state sort handling', () => {
+  test('Non-empty sort in URL should not fallback to state defaults', async () => {
+    history = createBrowserHistory();
+    history.push('/#?_a=(sort:!(!(order_date,desc)))');
+
+    state = getState({
+      getStateDefaults: () => ({ sort: [['fallback', 'desc']] }),
+      history,
+      uiSettings: uiSettingsMock,
+    });
+    await state.replaceUrlAppState({});
+    await state.startSync();
+    expect(state.appStateContainer.getState().sort).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "order_date",
+          "desc",
+        ],
+      ]
+    `);
+  });
+  test('Empty sort in URL should allow fallback state defaults', async () => {
+    history = createBrowserHistory();
+    history.push('/#?_a=(sort:!())');
+
+    state = getState({
+      getStateDefaults: () => ({ sort: [['fallback', 'desc']] }),
+      history,
+      uiSettings: uiSettingsMock,
+    });
+    await state.replaceUrlAppState({});
+    await state.startSync();
+    expect(state.appStateContainer.getState().sort).toMatchInlineSnapshot(`
+      Array [
+        Array [
+          "fallback",
+          "desc",
+        ],
+      ]
+    `);
+  });
+});
 
 describe('Test discover state with legacy migration', () => {
   test('migration of legacy query ', async () => {
diff --git a/src/plugins/discover/public/application/angular/discover_state.ts b/src/plugins/discover/public/application/angular/discover_state.ts
index e7d5ed469525f..f71e3ac651f53 100644
--- a/src/plugins/discover/public/application/angular/discover_state.ts
+++ b/src/plugins/discover/public/application/angular/discover_state.ts
@@ -170,6 +170,12 @@ export function getState({
     appStateFromUrl.query = migrateLegacyQuery(appStateFromUrl.query);
   }
 
+  if (appStateFromUrl?.sort && !appStateFromUrl.sort.length) {
+    // If there's an empty array given in the URL, the sort prop should be removed
+    // This allows the sort prop to be overwritten with the default sorting
+    delete appStateFromUrl.sort;
+  }
+
   let initialAppState = handleSourceColumnState(
     {
       ...defaultAppState,
@@ -177,7 +183,8 @@ export function getState({
     },
     uiSettings
   );
-  // todo filter source depending on fields fetchinbg flag (if no columns remain and source fetching is enabled, use default columns)
+
+  // todo filter source depending on fields fetching flag (if no columns remain and source fetching is enabled, use default columns)
   let previousAppState: AppState;
   const appStateContainer = createStateContainer<AppState>(initialAppState);
 
diff --git a/test/functional/apps/discover/_shared_links.ts b/test/functional/apps/discover/_shared_links.ts
index 2893102367b04..9522b665dd649 100644
--- a/test/functional/apps/discover/_shared_links.ts
+++ b/test/functional/apps/discover/_shared_links.ts
@@ -19,6 +19,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
   const browser = getService('browser');
   const toasts = getService('toasts');
   const deployment = getService('deployment');
+  const dataGrid = getService('dataGrid');
 
   describe('shared links', function describeIndexTests() {
     let baseUrl: string;
@@ -110,6 +111,32 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
           const actualUrl = await PageObjects.share.getSharedUrl();
           expect(actualUrl).to.be(expectedUrl);
         });
+
+        it('should load snapshot URL with empty sort param correctly', async function () {
+          const expectedUrl =
+            baseUrl +
+            '/app/discover?_t=1453775307251#' +
+            '/?_g=(filters:!(),refreshInterval:(pause:!t,value:0),time' +
+            ":(from:'2015-09-19T06:31:44.000Z',to:'2015-09" +
+            "-23T18:31:44.000Z'))&_a=(columns:!(),filters:!(),index:'logstash-" +
+            "*',interval:auto,query:(language:kuery,query:'')" +
+            ',sort:!())';
+          await browser.navigateTo(expectedUrl);
+          await PageObjects.discover.waitUntilSearchingHasFinished();
+          await retry.waitFor('url to contain default sorting', async () => {
+            // url fallback default sort should have been pushed to URL
+            const url = await browser.getCurrentUrl();
+            return url.includes('sort:!(!(%27@timestamp%27,desc))');
+          });
+
+          const row = await dataGrid.getRow({ rowIndex: 0 });
+          const firstRowText = await Promise.all(
+            row.map(async (cell) => await cell.getVisibleText())
+          );
+
+          // sorting requested by ES should be correct
+          expect(firstRowText).to.contain('Sep 22, 2015 @ 23:50:13.253');
+        });
       });
     });