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

Switch back to querystring-browser library #58943

Closed
wants to merge 9 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
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
2 changes: 1 addition & 1 deletion .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -343,7 +343,7 @@ module.exports = {
settings: {
// instructs import/no-extraneous-dependencies to treat certain modules
// as core modules, even if they aren't listed in package.json
'import/core-modules': ['plugins', 'legacy/ui', 'uiExports'],
'import/core-modules': ['plugins', 'legacy/ui', 'uiExports', 'querystring'],

'import/resolver': {
'@kbn/eslint-import-resolver-kibana': {
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@
"prop-types": "15.6.0",
"proxy-from-env": "1.0.0",
"pug": "^2.0.4",
"query-string": "6.10.1",
"querystring-browser": "1.0.4",
"raw-loader": "3.1.0",
"react": "^16.12.0",
"react-color": "^2.13.8",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ exports.getWebpackConfig = function(kibanaPath, projectRoot, config) {
// Kibana defaults https://github.com/elastic/kibana/blob/6998f074542e8c7b32955db159d15661aca253d7/src/legacy/ui/ui_bundler_env.js#L30-L36
ui: fromKibana('src/legacy/ui/public'),
test_harness: fromKibana('src/test_harness/public'),
querystring: 'querystring-browser',
Copy link
Contributor

@joshdover joshdover Mar 4, 2020

Choose a reason for hiding this comment

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

@spalger Is this acceptable? I think we would prefer not to introduce Webpack aliases. If this is just for the typings, I think we may be able to add a declare module 'querystring-browser' definition in typings/ that re-exports the types from Node's querystring to querystring-browser.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh this is the eslint config. Guess I'm not sure why this is necessary


// Dev defaults for test bundle https://github.com/elastic/kibana/blob/6998f074542e8c7b32955db159d15661aca253d7/src/core_plugins/tests_bundle/index.js#L73-L78
ng_mock$: fromKibana('src/test_utils/public/ng_mock'),
Expand Down
4 changes: 2 additions & 2 deletions src/core/server/http/http_server.mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
import { Request } from 'hapi';
import { merge } from 'lodash';
import { Socket } from 'net';
import { stringify } from 'query-string';
import querystring from 'querystring';

import { schema } from '@kbn/config-schema';

Expand Down Expand Up @@ -66,7 +66,7 @@ function createKibanaRequestMock<P = any, Q = any, B = any>({
validation = {},
kibanaRouteState = { xsrfRequired: true },
}: RequestFixtureOptions<P, Q, B> = {}) {
const queryString = stringify(query, { sort: false });
const queryString = querystring.stringify(query);

return KibanaRequest.from<P, Q, B>(
createRawRequestMock({
Expand Down
4 changes: 2 additions & 2 deletions src/core/utils/url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { ParsedQuery } from 'query-string';
import { ParsedUrlQuery } from 'querystring';
import { format as formatUrl, parse as parseUrl, UrlObject } from 'url';

/**
Expand All @@ -32,7 +32,7 @@ export interface URLMeaningfulParts {
protocol?: string | null;
slashes?: boolean | null;
port?: string | null;
query: ParsedQuery;
query: ParsedUrlQuery;
}

/**
Expand Down
4 changes: 2 additions & 2 deletions src/legacy/server/logging/log_format.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import Stream from 'stream';
import moment from 'moment-timezone';
import { get, _ } from 'lodash';
import queryString from 'query-string';
import querystring from 'querystring';
import numeral from '@elastic/numeral';
import chalk from 'chalk';
import stringify from 'json-stringify-safe';
Expand Down Expand Up @@ -108,7 +108,7 @@ export default class TransformObjStream extends Stream.Transform {
contentLength: contentLength,
};

const query = queryString.stringify(event.query, { sort: false });
const query = querystring.stringify(event.query);
if (query) data.req.url += '?' + query;

data.message = data.req.method.toUpperCase() + ' ';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { EuiFlexGroup, EuiFlexItem, EuiIcon, EuiScreenReaderOnly, EuiToolTip } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { debounce } from 'lodash';
import { parse } from 'query-string';
import { parse } from 'querystring';
import React, { CSSProperties, useCallback, useEffect, useRef, useState } from 'react';
// @ts-ignore
import mappings from '../../../../../lib/mappings/mappings';
Expand Down Expand Up @@ -101,7 +101,7 @@ function EditorUI({ initialTextValue }: EditorProps) {
const readQueryParams = () => {
const [, queryString] = (window.location.hash || '').split('?');

return parse(queryString || '', { sort: false }) as Required<QueryParams>;
return parse(queryString || '') as Required<QueryParams>;
};

const loadBufferFromRemote = (url: string) => {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/console/public/lib/es/es.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import $ from 'jquery';
import { stringify } from 'query-string';
import { stringify } from 'querystring';

const esVersion: string[] = [];

Expand All @@ -35,7 +35,7 @@ export function send(method: string, path: string, data: any) {
const wrappedDfd = $.Deferred(); // eslint-disable-line new-cap

const options: JQuery.AjaxSettings = {
url: '../api/console/proxy?' + stringify({ path, method }, { sort: false }),
url: '../api/console/proxy?' + stringify({ path, method }),
data,
contentType: getContentType(data),
cache: false,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { parse } from 'query-string';
import { parse } from 'querystring';

export function parseQueryString() {
// window.location.search is an empty string
Expand All @@ -26,5 +26,5 @@ export function parseQueryString() {
return {};
}

return parse(hrefSplit[1], { sort: false });
return parse(hrefSplit[1]);
}
23 changes: 23 additions & 0 deletions src/plugins/kibana_utils/common/url/encode_uri_query.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,27 @@ describe('encodeQuery', () => {
g: 'null',
});
});

test('encodeQuery without encoding', () => {
expect(
encodeQuery(
{
a: 'asdf1234asdf',
b: "-_.!~*'() -_.!~*'()",
c: ':@$, :@$,',
d: "&;=+# &;=+#'",
f: ' ',
g: 'null',
},
v => v
)
).toEqual({
a: 'asdf1234asdf',
b: "-_.!~*'() -_.!~*'()",
c: ':@$, :@$,',
d: "&;=+# &;=+#'",
f: ' ',
g: 'null',
});
});
});
12 changes: 9 additions & 3 deletions src/plugins/kibana_utils/common/url/encode_uri_query.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { ParsedQuery } from 'query-string';
import { stringify, ParsedUrlQuery } from 'querystring';
import { transform } from 'lodash';

/**
Expand All @@ -42,9 +42,9 @@ export function encodeUriQuery(val: string, pctEncodeSpaces = false) {
}

export const encodeQuery = (
query: ParsedQuery,
query: ParsedUrlQuery | {},
encodeFunction: (val: string, pctEncodeSpaces?: boolean) => string = encodeUriQuery
) =>
): ParsedUrlQuery =>
transform(query, (result, value, key) => {
if (key) {
const singleValue = Array.isArray(value) ? value.join(',') : value;
Expand All @@ -55,3 +55,9 @@ export const encodeQuery = (
);
}
});

export const stringifyWithEncoding = (query: ParsedUrlQuery | {}) =>
stringify(query, undefined, undefined, { encodeURIComponent: encodeUriQuery });

export const stringifyWithoutEncoding = (query: ParsedUrlQuery | {}) =>
stringify(query, undefined, undefined, { encodeURIComponent: (c: unknown) => c });
9 changes: 8 additions & 1 deletion src/plugins/kibana_utils/common/url/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,16 @@
* under the License.
*/

import { encodeUriQuery, encodeQuery } from './encode_uri_query';
import {
encodeUriQuery,
encodeQuery,
stringifyWithEncoding,
stringifyWithoutEncoding,
} from './encode_uri_query';

export const url = {
encodeQuery,
encodeUriQuery,
stringifyWithEncoding,
stringifyWithoutEncoding,
};
6 changes: 3 additions & 3 deletions src/plugins/kibana_utils/public/history/remove_query_param.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,18 @@
* under the License.
*/

import { parse, stringify } from 'query-string';
import { parse } from 'querystring';
import { History, Location } from 'history';
import { url } from '../../common';

export function removeQueryParam(history: History, param: string, replace: boolean = true) {
const oldLocation = history.location;
const search = (oldLocation.search || '').replace(/^\?/, '');
const query = parse(search, { sort: false });
const query = parse(search);

delete query[param];

const newSearch = stringify(url.encodeQuery(query), { sort: false, encode: false });
const newSearch = url.stringifyWithEncoding(query);
const newLocation: Location<any> = {
...oldLocation,
search: newSearch,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,18 @@
*/

import { format as formatUrl } from 'url';
import { stringify, ParsedQuery } from 'query-string';
import { ParsedUrlQuery } from 'querystring';
import { parseUrl, parseUrlHash } from './parse';
import { url as urlUtils } from '../../../common';

export function replaceUrlHashQuery(
rawUrl: string,
queryReplacer: (query: ParsedQuery) => ParsedQuery
queryReplacer: (query: ParsedUrlQuery) => ParsedUrlQuery
) {
const url = parseUrl(rawUrl);
const hash = parseUrlHash(rawUrl);
const newQuery = queryReplacer(hash?.query || {});
const searchQueryString = stringify(urlUtils.encodeQuery(newQuery), {
sort: false,
encode: false,
});
const searchQueryString = urlUtils.stringifyWithEncoding(newQuery);

if ((!hash || !hash.search) && !searchQueryString) return rawUrl; // nothing to change. return original url
return formatUrl({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
*/

import { format as formatUrl } from 'url';
import { stringify } from 'query-string';
import { createBrowserHistory, History } from 'history';
import { decodeState, encodeState } from '../state_encoder';
import { getCurrentUrl, parseUrl, parseUrlHash } from './parse';
Expand Down Expand Up @@ -244,11 +243,11 @@ export function getRelativeToHistoryPath(absoluteUrl: string, history: History):

return formatUrl({
pathname: stripBasename(parsedUrl.pathname),
search: stringify(urlUtils.encodeQuery(parsedUrl.query), { sort: false, encode: false }),
search: urlUtils.stringifyWithEncoding(parsedUrl.query),
hash: parsedHash
? formatUrl({
pathname: parsedHash.pathname,
search: stringify(urlUtils.encodeQuery(parsedHash.query), { sort: false, encode: false }),
search: urlUtils.stringifyWithEncoding(parsedHash.query),
})
: parsedUrl.hash,
});
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/timelion/server/series_functions/quandl.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { parse } from 'query-string';
import { parse } from 'querystring';
import fn from './quandl';
import moment from 'moment';
import fetchMock from 'node-fetch';
Expand All @@ -26,7 +26,7 @@ const parseURL = require('url').parse;
const tlConfig = require('./fixtures/tl_config')();

function parseUrlParams(url) {
return parse(parseURL(url).query, { sort: false });
return parse(parseURL(url).query);
}

jest.mock('node-fetch', () =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,21 +4,21 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse, stringify } from 'query-string';
import { parse, stringify } from 'querystring';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { LocalUIFilterName } from '../../../../../../../plugins/apm/server/lib/ui_filters/local_ui_filters/config';
import { url } from '../../../../../../../../src/plugins/kibana_utils/public';

export function toQuery(search?: string): APMQueryParamsRaw {
return search ? parse(search.slice(1), { sort: false }) : {};
return search ? parse(search.slice(1)) : {};
}

export function fromQuery(query: Record<string, any>) {
const encodedQuery = url.encodeQuery(query, value =>
encodeURIComponent(value).replace(/%3A/g, ':')
);

return stringify(encodedQuery, { sort: false, encode: false });
return url.stringifyWithEncoding(encodedQuery);
}

export type APMQueryParams = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse, stringify } from 'query-string';
import { parse, stringify } from 'querystring';
import React from 'react';
import { withRouter, RouteComponentProps } from 'react-router-dom';
import { FlatObject } from '../frontend_types';
Expand All @@ -31,9 +31,7 @@ export class WithURLStateComponent<URLState extends object> extends React.Compon
> {
private get URLState(): URLState {
// slice because parse does not account for the initial ? in the search string
return parse(decodeURIComponent(this.props.history.location.search).substring(1), {
sort: false,
}) as URLState;
return parse(decodeURIComponent(this.props.history.location.search).substring(1)) as URLState;
}

private historyListener: (() => void) | null = null;
Expand Down Expand Up @@ -65,13 +63,10 @@ export class WithURLStateComponent<URLState extends object> extends React.Compon
newState = state;
}

const search: string = stringify(
{
...pastState,
...newState,
},
{ sort: false }
);
const search: string = stringify({
...pastState,
...newState,
});

const newLocation = {
...this.props.history.location,
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/canvas/public/lib/app_state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { parse } from 'query-string';
import { parse } from 'querystring';
import { get } from 'lodash';
// @ts-ignore untyped local
import { getInitialState } from '../state/initial_state';
Expand Down Expand Up @@ -38,7 +38,7 @@ export function getDefaultAppState(): AppState {
export function getCurrentAppState(): AppState {
const history = historyProvider(getWindow());
const { search } = history.getLocation();
const qs = !!search ? parse(search.replace(/^\?/, ''), { sort: false }) : {};
const qs = !!search ? parse(search.replace(/^\?/, '')) : {};
const appState = assignAppState({}, qs);

return appState;
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/canvas/public/lib/modify_url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { ParsedQuery } from 'query-string';
import { ParsedUrlQuery } from 'querystring';
import { format as formatUrl, parse as parseUrl, UrlObject } from 'url';

/**
Expand All @@ -20,7 +20,7 @@ export interface URLMeaningfulParts {
protocol?: string | null;
slashes?: boolean | null;
port?: string | null;
query: ParsedQuery;
query: ParsedUrlQuery;
}

/**
Expand Down
Loading