From e6c2fb3a75b0262396611f84cf8d95727176ed2d Mon Sep 17 00:00:00 2001 From: Charley Kline Date: Thu, 20 Apr 2023 18:26:39 -0500 Subject: [PATCH] Coalesce toQueryString and query-string-encoding Closes FOO-3485 flag=none There were two separate shared utils for encoding a JavaScript object into a URL query string; this combines them both into a single place. There are now two ways to encode; they have slightly different type signatures (array of objects vs a single object) but one just calls the other after some massaging. We will prefer toQueryString() in new code as it does a much better job of matching the "PHP-style" query string encoding that our Rails code seems to prefer; there's a wrapper method for the legacy encodeQueryString one. Tests were also coalesced and converted to Jest, and more test cases were added for the kind of tricky legacy encodeQueryString method. Test plan: * automated tests pass Change-Id: Ib6f0246b3f438646967b233b72a2632ae2de9159 Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/316612 Reviewed-by: Yona Appletree Tested-by: Service Cloud Jenkins QA-Review: Charley Kline Product-Review: Charley Kline --- .../javascripts/jsx/shared/queryStringSpec.js | 47 --------- ui/shared/do-fetch-api-effect/index.ts | 14 ++- .../__tests__/index.test.ts | 99 +++++++++++++++++++ ui/shared/query-string-encoding/index.ts | 97 ++++++++++++++++-- .../util/__tests__/toQueryString.test.ts | 55 ----------- ui/shared/util/toQueryString.ts | 80 --------------- 6 files changed, 197 insertions(+), 195 deletions(-) delete mode 100644 spec/javascripts/jsx/shared/queryStringSpec.js create mode 100644 ui/shared/query-string-encoding/__tests__/index.test.ts delete mode 100644 ui/shared/util/__tests__/toQueryString.test.ts delete mode 100644 ui/shared/util/toQueryString.ts diff --git a/spec/javascripts/jsx/shared/queryStringSpec.js b/spec/javascripts/jsx/shared/queryStringSpec.js deleted file mode 100644 index 9d1968b9964..00000000000 --- a/spec/javascripts/jsx/shared/queryStringSpec.js +++ /dev/null @@ -1,47 +0,0 @@ -/* - * Copyright (C) 2017 - present Instructure, Inc. - * - * This file is part of Canvas. - * - * Canvas is free software: you can redistribute it and/or modify it under - * the terms of the GNU Affero General Public License as published by the Free - * Software Foundation, version 3 of the License. - * - * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY - * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR - * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more - * details. - * - * You should have received a copy of the GNU Affero General Public License along - * with this program. If not, see . - */ - -import {encodeQueryString, decodeQueryString} from '@canvas/query-string-encoding' - -QUnit.module('Query String util') - -QUnit.module('encodeQueryString') - -test('encodes a query string from array of params', () => { - const params = [{foo: 'bar'}, {hello: 'world'}] - const query = encodeQueryString(params) - equal(query, 'foo=bar&hello=world') -}) - -test('encodes a query string from array of params with duplicate keys', () => { - const params = [{'foo[]': 'bar'}, {'foo[]': 'world'}] - const query = encodeQueryString(params) - equal(query, 'foo[]=bar&foo[]=world') -}) - -QUnit.module('decodeQueryString') - -test('decodes a query string into an array of params', () => { - const params = decodeQueryString('foo=bar&hello=world') - deepEqual(params, [{foo: 'bar'}, {hello: 'world'}]) -}) - -test('decodes a query string with duplicate keys into an array of params', () => { - const params = decodeQueryString('foo[]=bar&foo[]=world') - deepEqual(params, [{'foo[]': 'bar'}, {'foo[]': 'world'}]) -}) diff --git a/ui/shared/do-fetch-api-effect/index.ts b/ui/shared/do-fetch-api-effect/index.ts index fa31834bce3..3762755a857 100644 --- a/ui/shared/do-fetch-api-effect/index.ts +++ b/ui/shared/do-fetch-api-effect/index.ts @@ -19,10 +19,16 @@ import getCookie from '@instructure/get-cookie' import parseLinkHeader from 'parse-link-header' import {defaultFetchOptions} from '@instructure/js-utils' -import toQueryString from '@canvas/util/toQueryString' -import type {QueryParameterMap} from '@canvas/util/toQueryString' +import {toQueryString} from '@canvas/query-string-encoding' +import type {QueryParameterRecord} from '@canvas/query-string-encoding' -function constructRelativeUrl({path, params}: {path: string; params: QueryParameterMap}): string { +function constructRelativeUrl({ + path, + params, +}: { + path: string + params: QueryParameterRecord +}): string { const queryString = toQueryString(params) if (queryString.length === 0) return path return path + '?' + queryString @@ -37,7 +43,7 @@ export type DoFetchApiOpts = { path: string method?: string headers?: {[k: string]: string} - params?: QueryParameterMap + params?: QueryParameterRecord // eslint-disable-next-line no-undef body?: BodyInit fetchOpts?: RequestInit diff --git a/ui/shared/query-string-encoding/__tests__/index.test.ts b/ui/shared/query-string-encoding/__tests__/index.test.ts new file mode 100644 index 00000000000..6f2c6e3e9fb --- /dev/null +++ b/ui/shared/query-string-encoding/__tests__/index.test.ts @@ -0,0 +1,99 @@ +/* + * Copyright (C) 2023 - present Instructure, Inc. + * + * This file is part of Canvas. + * + * Canvas is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3 of the License. + * + * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY + * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR + * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License along + * with this program. If not, see . + */ + +import {toQueryString, encodeQueryString, decodeQueryString} from '../index' +import type {QueryParameterRecord} from '../index' +import $ from 'jquery' + +type EncodeQueryStringParams = Array> + +const func = () => 'result' +const func2 = () => 'yet another result' + +const EX: {[k: string]: QueryParameterRecord} = { + empty: {}, + scalars_with_numbers_strings_and_bools: {frd: 1, str: 'syzygy', butNo: false}, + scalars_with_null: {frd: 1, butNo: null}, + scalars_with_undefined: {frd: 1, butNo: undefined, last: 'zzz'}, + scalars_with_functions: {frd: 1, func, func2}, + simple_array: {blah: [1, 2, 3]}, + mixed_types_array: {blah: [1, 'b', 'III']}, + recursive_object: {blah: 1, sub: {one: 1, two: [2, 4]}, last: 'zzz'}, + array_of_objects: { + blah: [ + {one: 1, two: 2}, + {one: '1', two: '2'}, + ], + }, + object_with_arrays: {blah: {one: [1, 2, 3], two: [2, 4, 6]}}, +} + +describe('toQueryString::', () => { + describe('does everything jQuery does', () => { + for (const ex in EX) { + it(`handles example ${ex}`, () => { + const subject = EX[ex] + const testResult = toQueryString(subject) + const jQueryResult = $.param(subject) + expect(testResult).toBe(jQueryResult) + }) + } + }) +}) + +describe('encodeQueryString::', () => { + it('encodes a query string from array of params', () => { + const params: EncodeQueryStringParams = [{foo: 'bar'}, {hello: 'world'}] + const query = encodeQueryString(params) + expect(query).toBe('foo=bar&hello=world') + }) + + it('encodes a query string from array of params with duplicate keys', () => { + const params: EncodeQueryStringParams = [{'foo[]': 'bar'}, {'foo[]': 'world'}] + const query = encodeQueryString(params) + expect(query).toBe('foo%5B%5D=bar&foo%5B%5D=world') + }) + + it('won’t pass along any null values for some reason', () => { + const params: EncodeQueryStringParams = [{foo: '256'}, {hello: null}, {baz: '512'}] + const query = encodeQueryString(params) + expect(query).toBe('foo=256&baz=512') + }) + + it('throws TypeError if we try to turn a non-array param into an array param', () => { + const params: EncodeQueryStringParams = [{foo: 'bar'}, {'foo[]': 'world'}] + expect(() => encodeQueryString(params)).toThrow(TypeError) + }) + + it('throws TypeError if we try to turn an array param into a non-array param', () => { + const params: EncodeQueryStringParams = [{'foo[]': 'bar'}, {foo: 'world'}] + expect(() => encodeQueryString(params)).toThrow(TypeError) + }) +}) + +describe('decodeQueryString::', () => { + it('decodes a query string into an array of params', () => { + const params = decodeQueryString('foo=bar&hello=world') + expect(params).toMatchObject([{foo: 'bar'}, {hello: 'world'}]) + }) + + it('decodes a query string with duplicate keys into an array of params', () => { + const params = decodeQueryString('foo[]=bar&foo[]=world') + expect(params).toMatchObject([{'foo[]': 'bar'}, {'foo[]': 'world'}]) + }) +}) diff --git a/ui/shared/query-string-encoding/index.ts b/ui/shared/query-string-encoding/index.ts index 8e233faab82..467605f9517 100644 --- a/ui/shared/query-string-encoding/index.ts +++ b/ui/shared/query-string-encoding/index.ts @@ -16,17 +16,96 @@ * with this program. If not, see . */ -export function encodeQueryString(params: Array<{[key: string]: string | null}>) { - return params - .map(param => { - const key = Object.keys(param)[0] - const value = param[key] - return value !== null ? `${key}=${value}` : null - }) - .filter(Boolean) - .join('&') +// toQueryString does what jQuery's .param() does, only without having to bring in +// all of jQuery. + +// URLSearchParams turns arrays into queries like a=1,2,3 +// But our jQuery makes the older "PHP" style like a[]=1&a[]=2&a[]=3 +// JQuery also magically deals with parameters that are functions, and recursive +// objects within objects, {a: 1, b: {c: 2, d: 3}, e: 4} => a=1&b[c]=2&b[d]=3&e=4 +// ... so we have to do a lot of massaging with the params object before using it +// So fun! + +type QueryParameterElement = + | string + | number + | boolean + | null + | undefined + | (() => string) // n.b. for jQuery compatibility, this does not expect a generic return + | Array + | QueryParameterRecord + +export type QueryParameterRecord = {[k: string]: QueryParameterElement} + +export function toQueryString(params: QueryParameterRecord): string { + const paramsWithIndexes: Array<[k: string, v: string]> = [] + + // fix up the array/object indexes to match the PHP standard + const fixIndexes = (elt: [string, string]): [string, string] => [ + elt[0] + .replace(/\[\d+\]$/, '[]') + .replace(/{/g, '[') + .replace(/}/g, ']'), + elt[1], + ] + + function serialize(k: string, elt: QueryParameterElement, suffix: string): void { + if (elt instanceof Function) { + paramsWithIndexes.push([k + suffix, elt()]) + } else if (elt instanceof Array) { + elt.forEach((k2, i) => serialize(k, k2, `${suffix}[${i}]`)) + } else if (elt instanceof Object) { + Object.keys(elt).forEach(k2 => serialize(k, elt[k2], `${suffix}{${k2}}`)) + } else if (typeof elt === 'boolean' || typeof elt === 'number') { + paramsWithIndexes.push([k + suffix, elt.toString()]) + } else if (typeof elt === 'undefined') { + paramsWithIndexes.push([k + suffix, 'undefined']) + } else if (elt === null) { + paramsWithIndexes.push([k + suffix, 'null']) + } else if (typeof elt === 'string') { + paramsWithIndexes.push([k + suffix, elt]) + } + } + + Object.keys(params).forEach(k => serialize(k, params[k], '')) + return new URLSearchParams(paramsWithIndexes.map(fixIndexes)).toString() } +// This is just to implement backward-compatibility from the old package. Almost +// nothing uses it anyway and we recommend toQueryString() for new needs. +// Need to be careful about duplicated keys, which have to be mapped into arrays. +export function encodeQueryString(params: Array>): string { + const realParms: QueryParameterRecord = {} + params.forEach(p => { + const k = Object.keys(p)[0] + const m = k.match(/(.+)\[\]/) + if (p[k] === null) return + if (m === null) { + // Not building up an array, just jam this in there, unless it's already + // in there and IS already an array. + if (Array.isArray(realParms[k])) throw new TypeError('cannot mix scalar and array parameters') + Object.assign(realParms, p) + } else { + // We ARE building up an array... append this value to an existing array + // if it exists, otherwise create one. If an existing key already exists + // and is NOT an array, then this is an error + const idx = m[1] + let arrayParm = realParms[idx] + if (typeof arrayParm === 'undefined') arrayParm = [] + if (!Array.isArray(arrayParm)) throw new TypeError('cannot mix scalar and array parameters') + arrayParm.push(p[k]) + realParms[idx] = arrayParm + } + }) + return toQueryString(realParms) +} + +// TODO: should be an inverse of toQueryString that takes a string and returns a +// QueryParameterRecord. The result would be slightly different from decodeQueryString +// in terms of how things like foo[]=1&foo[]=2 would be decoded. But it doesn't +// seem like any current usage cares about that kind of thing. + export function decodeQueryString(string: string) { return string .split('&') diff --git a/ui/shared/util/__tests__/toQueryString.test.ts b/ui/shared/util/__tests__/toQueryString.test.ts deleted file mode 100644 index 3c35a6af9c7..00000000000 --- a/ui/shared/util/__tests__/toQueryString.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -/* - * Copyright (C) 2022 - present Instructure, Inc. - * - * This file is part of Canvas. - * - * Canvas is free software: you can redistribute it and/or modify it under - * the terms of the GNU Affero General Public License as published by the Free - * Software Foundation, version 3 of the License. - * - * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY - * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR - * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more - * details. - * - * You should have received a copy of the GNU Affero General Public License along - * with this program. If not, see . - */ - -import toQueryString from '../toQueryString' -import type {QueryParameterMap} from '../toQueryString' -import $ from 'jquery' - -const func = () => 'result' -const func2 = () => 'yet another result' - -const EX: {[k: string]: QueryParameterMap} = { - empty: {}, - scalars_with_numbers_strings_and_bools: {frd: 1, str: 'syzygy', butNo: false}, - scalars_with_null: {frd: 1, butNo: null}, - scalars_with_undefined: {frd: 1, butNo: undefined, last: 'zzz'}, - scalars_with_functions: {frd: 1, func, func2}, - simple_array: {blah: [1, 2, 3]}, - mixed_types_array: {blah: [1, 'b', 'III']}, - recursive_object: {blah: 1, sub: {one: 1, two: [2, 4]}, last: 'zzz'}, - array_of_objects: { - blah: [ - {one: 1, two: 2}, - {one: '1', two: '2'}, - ], - }, - object_with_arrays: {blah: {one: [1, 2, 3], two: [2, 4, 6]}}, -} - -describe('toQueryString::', () => { - describe('does everything jQuery does', () => { - for (const ex in EX) { - it(`handles example ${ex}`, () => { - const subject = EX[ex] - const testResult = toQueryString(subject) - const jQueryResult = $.param(subject) - expect(testResult).toBe(jQueryResult) - }) - } - }) -}) diff --git a/ui/shared/util/toQueryString.ts b/ui/shared/util/toQueryString.ts deleted file mode 100644 index 13b25c04fd0..00000000000 --- a/ui/shared/util/toQueryString.ts +++ /dev/null @@ -1,80 +0,0 @@ -/* - * Copyright (C) 2022 - present Instructure, Inc. - * - * This file is part of Canvas. - * - * Canvas is free software: you can redistribute it and/or modify it under - * the terms of the GNU Affero General Public License as published by the Free - * Software Foundation, version 3 of the License. - * - * Canvas is distributed in the hope that it will be useful, but WITHOUT ANY - * WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR - * A PARTICULAR PURPOSE. See the GNU Affero General Public License for more - * details. - * - * You should have received a copy of the GNU Affero General Public License along - * with this program. If not, see . - */ - -// This does what jQuery's .param() does, only without having to bring in all -// of jQuery. - -// URLSearchParams turns arrays into queries like a=1,2,3 -// But our jQuery makes the older "PHP" style like a[]=1&a[]=2&a[]=3 -// JQuery also magically deals with parameters that are functions, and recursive -// objects within objects, {a: 1, b: {c: 2, d: 3}, e: 4} => a=1&b[c]=2&b[d]=3&e=4 -// ... so we have to do a lot of massaging with the params object before using it -// So fun! - -type QueryParameterElement = - | string - | number - | boolean - | null - | undefined - | (() => string) // n.b. for jQuery compatibility, this does not expect a generic return - | Array - | QueryParameterMap - -export type QueryParameterMap = {[k: string]: QueryParameterElement} - -export default function toQueryString(params: QueryParameterMap): string { - const paramsWithIndexes: Array<[k: string, v: string]> = [] - - // fix up the array/object indexes to match the PHP standard - function fixIndexes(elt: [string, string]): [string, string] { - return [ - elt[0] - .replace(/\[\d+\]$/, '[]') - .replace(/{/g, '[') - .replace(/}/g, ']'), - elt[1], - ] - } - - function serialize(k: string, elt: QueryParameterElement, suffix: string): void { - if (elt instanceof Function) { - paramsWithIndexes.push([k + suffix, elt()]) - } else if (elt instanceof Array) { - for (const k2 in elt) { - serialize(k, elt[k2], `${suffix}[${k2}]`) - } - } else if (elt instanceof Object) { - for (const k2 in elt) { - serialize(k, elt[k2], `${suffix}{${k2}}`) - } - } else if (typeof elt === 'boolean' || typeof elt === 'number') { - paramsWithIndexes.push([k + suffix, elt.toString()]) - } else if (typeof elt === 'undefined') { - paramsWithIndexes.push([k + suffix, 'undefined']) - } else if (elt === null) { - paramsWithIndexes.push([k + suffix, 'null']) - } else if (typeof elt === 'string') { - paramsWithIndexes.push([k + suffix, elt]) - } - } - - for (const k in params) serialize(k, params[k], '') - - return new URLSearchParams(paramsWithIndexes.map(fixIndexes)).toString() -}