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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Feb 15, 2022
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
76ce8b1
fix errors with rison and useQueryParams
Jan 14, 2022
d88c4fe
add test for encode/decode
Feb 3, 2022
eee8975
add rison link and make test case more readable
Feb 15, 2022
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
3 changes: 1 addition & 2 deletions superset-frontend/src/components/ListView/Filters/Search.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,7 @@ export default function SearchFilter({
const [value, setValue] = useState(initialValue || '');
const handleSubmit = () => {
if (value) {
// encode plus signs to prevent them from being converted into a space
onSubmit(value.trim().replace(/\+/g, '%2B'));
onSubmit(value.trim());
}
};
const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => {
Expand Down
11 changes: 9 additions & 2 deletions superset-frontend/src/components/ListView/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,10 +46,17 @@ import {
} from './types';

// Define custom RisonParam for proper encoding/decoding; note that
// plus symbols should be encoded to avoid being converted into a space
// %, &, +, and # must be encoded to avoid breaking the url
const RisonParam: QueryParamConfig<string, any> = {
encode: (data?: any | null) =>
data === undefined ? undefined : rison.encode(data).replace(/\+/g, '%2B'),
data === undefined
? undefined
: rison
.encode(data)
.replace(/%/g, '%25')
.replace(/&/g, '%26')
.replace(/\+/g, '%2B')
.replace(/#/g, '%23'),
decode: (dataStr?: string | string[]) =>
dataStr === undefined || Array.isArray(dataStr)
? undefined
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterLabel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ const fetchTimeRange = async (
timeRange: string,
endpoints?: TimeRangeEndpoints,
) => {
const query = rison.encode(timeRange);
const query = rison.encode_uri(timeRange);
const endpoint = `/api/v1/time_range/?q=${query}`;
try {
const response = await SupersetClient.get({ endpoint });
Expand Down
4 changes: 2 additions & 2 deletions superset-frontend/src/views/CRUD/alert/AlertReportModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
const loadDashboardOptions = useMemo(
() =>
(input = '', page: number, pageSize: number) => {
const query = rison.encode({
const query = rison.encode_uri({
filter: input,
page,
page_size: pageSize,
Expand Down Expand Up @@ -748,7 +748,7 @@ const AlertReportModal: FunctionComponent<AlertReportModalProps> = ({
const loadChartOptions = useMemo(
() =>
(input = '', page: number, pageSize: number) => {
const query = rison.encode({
const query = rison.encode_uri({
filter: input,
page,
page_size: pageSize,
Expand Down
2 changes: 1 addition & 1 deletion superset-frontend/src/views/CRUD/hooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export function useListViewResource<D extends object = any>(
: value,
}));

const queryParams = rison.encode({
const queryParams = rison.encode_uri({
order_column: sortBy[0].id,
order_direction: sortBy[0].desc ? 'desc' : 'asc',
page: pageIndex,
Expand Down
19 changes: 19 additions & 0 deletions superset-frontend/src/views/CRUD/utils.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import rison from 'rison';
import {
isNeedsPassword,
isAlreadyExists,
Expand Down Expand Up @@ -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);
});
31 changes: 30 additions & 1 deletion superset-frontend/src/views/CRUD/utils.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,35 @@ import { FetchDataConfig } from 'src/components/ListView';
import SupersetText from 'src/utils/textUtils';
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

(() => {
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;
id_ok: RegExp;
next_id: RegExp;
} = rison as any;

const l = [];
for (let hi = 0; hi < 16; hi += 1) {
for (let lo = 0; lo < 16; lo += 1) {
if (hi + lo === 0) continue;
const c = String.fromCharCode(hi * 16 + lo);
if (!/\w|[-_./~]/.test(c))
l.push(`\\u00${hi.toString(16)}${lo.toString(16)}`);
}
}

risonRef.not_idchar = l.join('');
risonRef.not_idstart = '-0123456789';

const idrx = `[^${risonRef.not_idstart}${risonRef.not_idchar}][^${risonRef.not_idchar}]*`;

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

const createFetchResourceMethod =
(method: string) =>
(
Expand All @@ -43,7 +72,7 @@ const createFetchResourceMethod =
) =>
async (filterValue = '', page: number, pageSize: number) => {
const resourceEndpoint = `/api/v1/${resource}/${method}/${relation}`;
const queryParams = rison.encode({
const queryParams = rison.encode_uri({
filter: filterValue,
page,
page_size: pageSize,
Expand Down