Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(CRUD/listviews): Errors with rison and search strings using special characters #18056

Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
add test for encode/decode
  • Loading branch information
Corbin Robb authored and Corbin Robb committed Feb 3, 2022
commit d88c4fed1e03dff55f3668f3cc399966c5834e76
19 changes: 19 additions & 0 deletions superset-frontend/src/views/CRUD/utils.test.tsx
Original file line number Diff line number Diff line change
@@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import rison from 'rison';
import {
isNeedsPassword,
isAlreadyExists,
@@ -171,3 +172,21 @@ test('does not ask for password when the import type is wrong', () => {
};
expect(hasTerminalValidation(error.errors)).toBe(true);
});

test('successfully modified rison to encode correctly', () => {
const problemCharacters = '& # ? ^ { } [ ] | " = + `';

const testObject = problemCharacters.split(' ').reduce((a, c) => {
// eslint-disable-next-line no-param-reassign
a[c] = c;
return a;
}, {});

const actualEncoding = rison.encode(testObject);

const expectedEncoding =
"('\"':'\"','#':'#','&':'&','+':'+','=':'=','?':'?','[':'[',']':']','^':'^','`':'`','{':'{','|':'|','}':'}')";
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this more readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can have it make and encode an object for each one of the characters being tested instead of having it all in one. It will get rid of this big string and should be easier to read. I am pushing it up now so let me know if it needs more work and I can give it another go


expect(actualEncoding).toEqual(expectedEncoding);
expect(rison.decode(actualEncoding)).toEqual(testObject);
});
6 changes: 2 additions & 4 deletions superset-frontend/src/views/CRUD/utils.tsx
Original file line number Diff line number Diff line change
@@ -35,7 +35,7 @@ import { Dashboard, Filters } from './types';

// Modifies the rison encoding slightly to match the backend's
// rison encoding/decoding. Applies globally. Code pulled from rison.js
Copy link
Member

Choose a reason for hiding this comment

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

Nit, can you add here that rison.js is licensed under the MIT license, so we know we can reuse it? We also need to have a link to the original project, since the MIT license requires crediting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing! I will have that pushed up soon

const fixRisonEncodeDecode = () => {
(() => {
Copy link
Member

Choose a reason for hiding this comment

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

is this called everytime rison.encode_uri is called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function should only run once and since it doesn't get named, exported, or called anywhere else, it shouldn't run again. This just takes advantage of the rison object being global and mutable, and changes the regex variable on it that decides what to do with certain characters when it encodes/decodes. That regex variable gets used every time encode_uri, encode or decode is called but this function itself doesn't

const risonRef: {
not_idchar: string;
not_idstart: string;
@@ -60,9 +60,7 @@ const fixRisonEncodeDecode = () => {

risonRef.id_ok = new RegExp(`^${idrx}$`);
risonRef.next_id = new RegExp(idrx, 'g');
};

fixRisonEncodeDecode();
})();

const createFetchResourceMethod =
(method: string) =>