[superset] branch master updated: fix: [sc-54864] Adds safety check to provide near term fix to save query (#21034)

[el...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

elizabeth pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git


The following commit(s) were added to refs/heads/master by this push:
     new ab6ec89f68 fix: [sc-54864] Adds safety check to provide near term fix to save query  (#21034)
ab6ec89f68 is described below

commit ab6ec89f680dbf022a39ed568c6fcdce0439b2dd
Author: Eric Briscoe <er...@gmail.com>
AuthorDate: Wed Aug 10 10:33:47 2022 -0700

    fix: [sc-54864] Adds safety check to provide near term fix to save query  (#21034)
    
    * [sc-54864] Adds safety check to provide near term fix for Queries not saving
    
    There is an error when casting the columns array to String() for saving queries where the objects in the array are missing the toString method.   This is a near term rapid patch to fix workflow in production which will have a follow up to identify root cause.
    
    * fix typo
    
    Co-authored-by: Elizabeth Thompson <es...@gmail.com>
    
    * Adjusted chekc to be explicit for undefined instead of truthy
    
    This fixes issue caught by unit test where the if statement was using a truthy check where it should be explicitly checking for value of undefined
    
    * Adds new unit test to get 100% coverage for callApi
    
    Co-authored-by: Elizabeth Thompson <es...@gmail.com>
---
 .../src/connection/callApi/callApi.ts              | 21 +++++++++--
 .../test/connection/callApi/callApi.test.ts        | 44 ++++++++++++++++++++++
 superset-frontend/src/SqlLab/actions/sqlLab.js     |  9 +++--
 3 files changed, 67 insertions(+), 7 deletions(-)

diff --git a/superset-frontend/packages/superset-ui-core/src/connection/callApi/callApi.ts b/superset-frontend/packages/superset-ui-core/src/connection/callApi/callApi.ts
index 7c3fe21fdb..7a19e94c03 100644
--- a/superset-frontend/packages/superset-ui-core/src/connection/callApi/callApi.ts
+++ b/superset-frontend/packages/superset-ui-core/src/connection/callApi/callApi.ts
@@ -146,10 +146,23 @@ export default async function callApi({
         Object.keys(payload).forEach(key => {
           const value = (payload as JsonObject)[key] as JsonValue;
           if (typeof value !== 'undefined') {
-            formData.append(
-              key,
-              stringify ? JSON.stringify(value) : String(value),
-            );
+            let valueString;
+            try {
+              // We have seen instances where casting to String() throws error
+              // This check allows all valid attributes to be appended to the formData
+              // while logging error to console for any attribute that fails the cast to String
+              valueString = stringify ? JSON.stringify(value) : String(value);
+            } catch (e) {
+              // eslint-disable-next-line no-console
+              console.error(
+                `Unable to convert attribute '${key}' to a String(). '${key}' was not added to the formData in request.body for call to ${url}`,
+                value,
+                e,
+              );
+            }
+            if (valueString !== undefined) {
+              formData.append(key, valueString);
+            }
           }
         });
         request.body = formData;
diff --git a/superset-frontend/packages/superset-ui-core/test/connection/callApi/callApi.test.ts b/superset-frontend/packages/superset-ui-core/test/connection/callApi/callApi.test.ts
index 81467ce239..81c8e2d150 100644
--- a/superset-frontend/packages/superset-ui-core/test/connection/callApi/callApi.test.ts
+++ b/superset-frontend/packages/superset-ui-core/test/connection/callApi/callApi.test.ts
@@ -23,6 +23,12 @@ import callApi from '../../../src/connection/callApi/callApi';
 
 import { LOGIN_GLOB } from '../fixtures/constants';
 
+// missing the toString function causing method to error out when casting to String
+class BadObject {}
+const corruptObject = new BadObject();
+/* @ts-expect-error */
+BadObject.prototype.toString = undefined;
+
 describe('callApi()', () => {
   beforeAll(() => {
     fetchMock.get(LOGIN_GLOB, { result: '1234' });
@@ -178,6 +184,44 @@ describe('callApi()', () => {
         expect(jsonRequestBody[key]).toEqual(value);
       });
     });
+
+    it('removes corrupt value when building formData with stringify = false', async () => {
+      /*
+        There has been a case when 'stringify' is false an object value on one of the
+        attributes was missing a toString function making the cast to String() fail
+        and causing entire method call to fail.  The new logic skips corrupt values that fail cast to String()
+        and allows all valid attributes to be added as key / value pairs to the formData
+        instance.  This test case replicates a corrupt object missing the .toString method
+        representing a real bug report.
+      */
+      const postPayload = {
+        string: 'value',
+        number: 1237,
+        array: [1, 2, 3],
+        object: { a: 'a', 1: 1 },
+        null: null,
+        emptyString: '',
+        // corruptObject has no toString method and will fail cast to String()
+        corrupt: [corruptObject],
+      };
+      jest.spyOn(console, 'error').mockImplementation();
+
+      await callApi({
+        url: mockPostUrl,
+        method: 'POST',
+        postPayload,
+        stringify: false,
+      });
+
+      const calls = fetchMock.calls(mockPostUrl);
+      expect(calls).toHaveLength(1);
+      const unstringified = (calls[0][1] as RequestInit).body as FormData;
+      const hasCorruptKey = unstringified.has('corrupt');
+      expect(hasCorruptKey).toBeFalsy();
+      // When a corrupt attribute is encountred, a console.error call is made with info about the corrupt attribute
+      // eslint-disable-next-line no-console
+      expect(console.error).toHaveBeenCalledTimes(1);
+    });
   });
 
   describe('PUT requests', () => {
diff --git a/superset-frontend/src/SqlLab/actions/sqlLab.js b/superset-frontend/src/SqlLab/actions/sqlLab.js
index 1435e0d796..6391ce82c9 100644
--- a/superset-frontend/src/SqlLab/actions/sqlLab.js
+++ b/superset-frontend/src/SqlLab/actions/sqlLab.js
@@ -908,9 +908,12 @@ export function updateSavedQuery(query) {
         dispatch(addSuccessToast(t('Your query was updated')));
         dispatch(queryEditorSetTitle(query, query.name));
       })
-      .catch(() =>
-        dispatch(addDangerToast(t('Your query could not be updated'))),
-      )
+      .catch(e => {
+        const message = t('Your query could not be updated');
+        // eslint-disable-next-line no-console
+        console.error(message, e);
+        dispatch(addDangerToast(message));
+      })
       .then(() => dispatch(updateQueryEditor(query)));
 }