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

[ui/utils/query_string]: Remove unused methods & migrate apps to querystring lib #56957

Merged
merged 35 commits into from Feb 12, 2020

Conversation

@alexwizp
Copy link
Collaborator

alexwizp commented Feb 6, 2020

Closes: #56704

Summary

What was done:

  1. querystring-browser dependency was removed
    This library is no longer being updated. We cannot use it in TypeScript code without @ ts-ignore, because there are no types available for it.

  2. query-string library was added instead of querystring . All import from querystring/querystring-browser were replaced to use new library

  3. kibana_utils/public/url service was introduced. Now we stringify all objects using one encodeQueryComponent method.

  4. encodeQueryComponent method was unified. Before that we had 3 different copies of that method. All duplicated were removed

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@alexwizp alexwizp force-pushed the alexwizp:56704 branch 2 times, most recently from 7f87b9a to 91a09cc Feb 6, 2020
@alexwizp alexwizp changed the title replace querystring (querystring-browser) -> query-string [ui/utils/query_string]: Remove unused methods & migrate apps to querystring lib Feb 6, 2020
@elastic elastic deleted a comment from kibanamachine Feb 6, 2020
@alexwizp alexwizp self-assigned this Feb 6, 2020
@elasticmachine

This comment has been minimized.

Copy link
Contributor

elasticmachine commented Feb 7, 2020

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elastic elastic deleted a comment from elasticmachine Feb 7, 2020
@alexwizp alexwizp force-pushed the alexwizp:56704 branch from a56e0d4 to c0950a2 Feb 7, 2020
@elastic elastic deleted a comment from elasticmachine Feb 7, 2020
@elastic elastic deleted a comment from elasticmachine Feb 7, 2020
@alexwizp alexwizp force-pushed the alexwizp:56704 branch from c0950a2 to d673105 Feb 7, 2020
alexwizp added 2 commits Feb 7, 2020
# Conflicts:
#	x-pack/legacy/plugins/rollup/server/routes/api/index_patterns.js
#	x-pack/test/api_integration/apis/management/rollup/index_patterns_extensions.js
@elastic elastic deleted a comment from elasticmachine Feb 7, 2020
@elastic elastic deleted a comment from elasticmachine Feb 7, 2020
@elastic elastic deleted a comment from kibanamachine Feb 9, 2020
elasticmachine and others added 2 commits Feb 9, 2020
Copy link
Contributor

lizozom left a comment

Overall looks good. Didn't test yet.
Added couple of questions.

@@ -34,5 +34,6 @@ export function encodeQueryComponent(val: string, pctEncodeSpaces = false) {
.replace(/%3A/gi, ':')
.replace(/%24/g, '$')
.replace(/%2C/gi, ',')
.replace(/%3B/gi, ';')

This comment has been minimized.

Copy link
@lizozom

lizozom Feb 9, 2020

Contributor

This seems like an actual logic change. Why was this added?

@@ -71,11 +71,11 @@ function getPdfUrlParts(

export function getPdfUrl(...args: Arguments): string {
const urlParts = getPdfUrlParts(...args);
const param = (key: string, val: any) =>

This comment has been minimized.

Copy link
@lizozom

lizozom Feb 9, 2020

Contributor

Why not add the param function to the URL utils?

This comment has been minimized.

Copy link
@alexwizp

alexwizp Feb 10, 2020

Author Collaborator

We use this method only in one place. Not sure that we need it somewhere else

@@ -443,7 +443,7 @@ export default function({ getService }: FtrProviderContext) {
it('should invalidate access token on IdP initiated logout', async () => {
const logoutRequest = await createLogoutRequest({ sessionIndex: idpSessionIndex });
const logoutResponse = await supertest
.get(`/api/security/logout?${querystring.stringify(logoutRequest)}`)
.get(`/api/security/logout?${stringify(logoutRequest, { sort: false })}`)

This comment has been minimized.

Copy link
@lizozom

lizozom Feb 9, 2020

Contributor

In other places if seems you replaced stringing with url.stringifyUrlQuery. why isn't this the case here?

This comment has been minimized.

Copy link
@alexwizp

alexwizp Feb 10, 2020

Author Collaborator

You are right, I did it in almost all places instead functional tests and kibana-core code. Not sure that we should add import from 'kibana_utils' from that places.

@elastic elastic deleted a comment from kibanamachine Feb 10, 2020
Copy link
Member

jasonrhodes left a comment

LGTM thanks for updating

import { useKibana } from '../../../../../../../../src/plugins/kibana_react/public';
import { TimeRange } from '../../../../common/http_api/shared/time_range';
import { url as urlUtils } from '../../../../../../../../src/plugins/kibana_utils/public';

This comment has been minimized.

Copy link
@jasonrhodes

jasonrhodes Feb 11, 2020

Member

Someday soon I hope we figure out a way to cleanly avoid this because it's awful to look at and horribly error-prone :(

This comment has been minimized.

Copy link
@lukeelmers

lukeelmers Feb 11, 2020

Member

This is the relevant PR to check out for that if you aren't already following it #52995 :)

@jasonrhodes

This comment has been minimized.

Copy link
Member

jasonrhodes commented Feb 11, 2020

Quick question: can someone outline why query-string was chosen over querystring? I only ask because server side, we shouldn't need a library for this, because node core ships with a "querystring" package. My understanding is that querystring the library provides the same exact API for the client. I don't feel strongly here but just wanted to hear what the idea was, for posterity.

Copy link
Contributor

poffdeluxe left a comment

Canvas changes lgtm

@alexwizp

This comment has been minimized.

Copy link
Collaborator Author

alexwizp commented Feb 12, 2020

@elasticmachine merge upstream

@kertal
kertal approved these changes Feb 12, 2020
Copy link
Contributor

kertal left a comment

Code LGTM, just took a look at the single timelion test of the KibanaApp team Codeowner realm

@alexwizp

This comment has been minimized.

Copy link
Collaborator Author

alexwizp commented Feb 12, 2020

@jgowdyelastic Could you please have a look? I need a review from ml-ui team

Copy link
Member

jgowdyelastic left a comment

ML changes LGTM

@alexwizp

This comment has been minimized.

Copy link
Collaborator Author

alexwizp commented Feb 12, 2020

@elasticmachine merge upstream

elasticmachine and others added 2 commits Feb 12, 2020
# Conflicts:
#	x-pack/legacy/plugins/siem/public/components/url_state/helpers.ts
@alexwizp alexwizp requested a review from sqren Feb 12, 2020
@lizozom lizozom requested a review from elastic/apm-ui Feb 12, 2020
) =>
transform(query, (result, value, key) => {
if (key) {
const singleValue = Array.isArray(value) ? value.join(',') : value;

This comment has been minimized.

Copy link
@sqren

sqren Feb 12, 2020

Member

Isn't this the same as arrayFormat: 'comma'?

queryString.stringify({ foo: [1, 2, 3] }, { arrayFormat: 'comma'} );
//=> 'foo=1,2,3'

This comment has been minimized.

Copy link
@alexwizp

alexwizp Feb 12, 2020

Author Collaborator

@sqren yes, if you don't need to encode your object using encodeQuery method you can definitely use { arrayFormat: 'comma'}. Now you can do it cause all plugins use query-string library directly without any wrappers. Thank you for your previous suggestion.

But I added this line, because initially the encodeUriQuery method worked only with the string type, but should also work fine with string arrays.

If you have any suggestions how to modify that code to use { arrayFormat: 'comma'} please let me know.

});
const encodedQuery = url.encodeQuery(query, value =>
encodeURIComponent(value).replace(/%3A/g, ':')
);

This comment has been minimized.

Copy link
@sqren

sqren Feb 12, 2020

Member

Thanks for making the changes above!

I think that perhaps url.encodeQuery changes the format of the encoded values slightly. This is probably not a problem, but I'd like the change to be explicit.

I've added a few specs to capture previous behaviour. Can you do me a favour and add them to x-pack/legacy/plugins/apm/public/components/shared/Links/url_helpers.test.tsx:

  it('should not encode the following characters', () => {
    expect(
      fromQuery({
        a: true,
        b: 5000,
        c: ':'
      })
    ).toEqual('a=true&b=5000&c=:');
  });

  it('should encode the following characters', () => {
    expect(
      fromQuery({
        a: '@',
        b: '.',
        c: ';',
        d: ' '
      })
    ).toEqual('a=%40&b=.&c=%3B&d=%20');
  });

  it('should handle null and undefined', () => {
    expect(
      fromQuery({
        a: undefined,
        b: null
      })
    ).toEqual('a=&b=');
  });

  it('should handle arrays', () => {
    expect(
      fromQuery({
        arr: ['a', 'b']
      })
    ).toEqual('arr=a&arr=b');
  });

That should make any discrepancies between the old and new behaviour clear.
Most notably I think the new encoding changes the array format, and handling of null and undefined.

This comment has been minimized.

Copy link
@alexwizp

alexwizp Feb 12, 2020

Author Collaborator

@sqren tests were added

@sqren
sqren approved these changes Feb 12, 2020
kibana-app-arch automation moved this from In progress to Review in progress Feb 12, 2020
@kibanamachine

This comment has been minimized.

Copy link

kibanamachine commented Feb 12, 2020

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@alexwizp alexwizp merged commit deda49e into elastic:master Feb 12, 2020
55 checks passed
55 checks passed
API integration tests node scripts/functional_tests --config test/api_integration/config.js --bail --debug
Details
Browser tests yarn run grunt test:browser-ci
Details
Build kbn_tp_sample_panel_action yarn build
Details
CLA All commits in pull request signed
Details
Check core API changes node scripts/check_core_api_changes
Details
Check file casing node scripts/check_file_casing --quiet
Details
Check licenses node scripts/check_licenses --dev
Details
Check lockfile symlinks node scripts/check_lockfile_symlinks --quiet
Details
Example functional tests node scripts/functional_tests --config test/examples/config.js --bail --debug
Details
Firefox smoke test node scripts/functional_tests --bail --debug --kibana-install-dir /dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64 --include-tag smoke --config test/functional/config.firefox.js
Details
Functional tests / Group 1 yarn run grunt run:functionalTests_ciGroup1
Details
Functional tests / Group 10 yarn run grunt run:functionalTests_ciGroup10
Details
Functional tests / Group 11 yarn run grunt run:functionalTests_ciGroup11
Details
Functional tests / Group 12 yarn run grunt run:functionalTests_ciGroup12
Details
Functional tests / Group 2 yarn run grunt run:functionalTests_ciGroup2
Details
Functional tests / Group 3 yarn run grunt run:functionalTests_ciGroup3
Details
Functional tests / Group 4 yarn run grunt run:functionalTests_ciGroup4
Details
Functional tests / Group 5 yarn run grunt run:functionalTests_ciGroup5
Details
Functional tests / Group 6 yarn run grunt run:functionalTests_ciGroup6
Details
Functional tests / Group 7 yarn run grunt run:functionalTests_ciGroup7
Details
Functional tests / Group 8 yarn run grunt run:functionalTests_ciGroup8
Details
Functional tests / Group 9 yarn run grunt run:functionalTests_ciGroup9
Details
Internationalization check node scripts/i18n_check --ignore-missing
Details
Interpreter functional tests node scripts/functional_tests --config test/interpreter_functional/config.ts --bail --debug --kibana-install-dir /dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64-2
Details
Jest integration tests yarn run grunt test:jest_integration
Details
Jest tests yarn run grunt test:jest
Details
Kibana accessibility tests node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/kibana/build/oss/kibana-8.0.0-SNAPSHOT-linux-x86_64 --config test/accessibility/config.ts
Details
Mocha tests node scripts/mocha
Details
Plugin functional tests node scripts/functional_tests --config test/plugin_functional/config.js --bail --debug
Details
Project tests yarn run grunt test:projects
Details
Type check node scripts/type_check
Details
TypeScript - all files belong to a TypeScript project node scripts/check_ts_projects
Details
Verify NOTICE.txt node scripts/notice --validate
Details
Verify dependency versions yarn run grunt verifyDependencyVersions
Details
X-Pack Chrome Functional tests / Group 1 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-2 --include-tag ciGroup1
Details
X-Pack Chrome Functional tests / Group 10 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-11 --include-tag ciGroup10
Details
X-Pack Chrome Functional tests / Group 2 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-3 --include-tag ciGroup2
Details
X-Pack Chrome Functional tests / Group 3 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-4 --include-tag ciGroup3
Details
X-Pack Chrome Functional tests / Group 4 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-5 --include-tag ciGroup4
Details
X-Pack Chrome Functional tests / Group 5 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-6 --include-tag ciGroup5
Details
X-Pack Chrome Functional tests / Group 6 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-7 --include-tag ciGroup6
Details
X-Pack Chrome Functional tests / Group 7 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-8 --include-tag ciGroup7
Details
X-Pack Chrome Functional tests / Group 8 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-9 --include-tag ciGroup8
Details
X-Pack Chrome Functional tests / Group 9 node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-10 --include-tag ciGroup9
Details
X-Pack Jest node scripts/jest --ci --verbose
Details
X-Pack Karma Tests yarn test:browser
Details
X-Pack SIEM cyclic dependency test node legacy/plugins/siem/scripts/check_circular_deps
Details
X-Pack accessibility tests node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana --config test/accessibility/config.ts
Details
X-Pack firefox smoke test node scripts/functional_tests --debug --bail --kibana-install-dir /dev/shm/workspace/install/kibana-1 --include-tag smoke --config test/functional/config.firefox.js
Details
eslint node scripts/eslint --no-cache
Details
kibana-ci Build finished.
Details
prbot:outdated
prbot:release note labels
prbot:release version labels
sasslint node scripts/sasslint
Details
kibana-app-arch automation moved this from Review in progress to Done in current release Feb 12, 2020
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 12, 2020
…ystring lib (elastic#56957)

* replace querystring (querystring-browser) -> query-string

* QueryString remove encode/decode methods

* remove query_string file

* remove querystring-browser from package.json

* add kibana_utils\url module

* cleanup

* update notice.txt

* fix merge conflict

* fix CI

* fix wrong import

* fix CI

* fix X-Pack firefox smoke test

* remove urlUtils.parseUrlQuery

* remove url.stringifyUrlQuery

* use url.encodeQuery

* Record<string, any> -> ParsedQuery

* Update src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx

Co-Authored-By: Luke Elmers <lukeelmers@gmail.com>

* add more tests for APM

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts:
#	x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 12, 2020
…ystring lib (elastic#56957)

* replace querystring (querystring-browser) -> query-string

* QueryString remove encode/decode methods

* remove query_string file

* remove querystring-browser from package.json

* add kibana_utils\url module

* cleanup

* update notice.txt

* fix merge conflict

* fix CI

* fix wrong import

* fix CI

* fix X-Pack firefox smoke test

* remove urlUtils.parseUrlQuery

* remove url.stringifyUrlQuery

* use url.encodeQuery

* Record<string, any> -> ParsedQuery

* Update src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx

Co-Authored-By: Luke Elmers <lukeelmers@gmail.com>

* add more tests for APM

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts:
#	x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 13, 2020
…ystring lib (elastic#56957)

* replace querystring (querystring-browser) -> query-string

* QueryString remove encode/decode methods

* remove query_string file

* remove querystring-browser from package.json

* add kibana_utils\url module

* cleanup

* update notice.txt

* fix merge conflict

* fix CI

* fix wrong import

* fix CI

* fix X-Pack firefox smoke test

* remove urlUtils.parseUrlQuery

* remove url.stringifyUrlQuery

* use url.encodeQuery

* Record<string, any> -> ParsedQuery

* Update src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx

Co-Authored-By: Luke Elmers <lukeelmers@gmail.com>

* add more tests for APM

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts:
#	x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts
alexwizp added a commit to alexwizp/kibana that referenced this pull request Feb 13, 2020
…ystring lib (elastic#56957)

* replace querystring (querystring-browser) -> query-string

* QueryString remove encode/decode methods

* remove query_string file

* remove querystring-browser from package.json

* add kibana_utils\url module

* cleanup

* update notice.txt

* fix merge conflict

* fix CI

* fix wrong import

* fix CI

* fix X-Pack firefox smoke test

* remove urlUtils.parseUrlQuery

* remove url.stringifyUrlQuery

* use url.encodeQuery

* Record<string, any> -> ParsedQuery

* Update src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx

Co-Authored-By: Luke Elmers <lukeelmers@gmail.com>

* add more tests for APM

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts:
#	x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts
alexwizp added a commit that referenced this pull request Feb 13, 2020
…ystring lib (#56957) (#57533)

* replace querystring (querystring-browser) -> query-string

* QueryString remove encode/decode methods

* remove query_string file

* remove querystring-browser from package.json

* add kibana_utils\url module

* cleanup

* update notice.txt

* fix merge conflict

* fix CI

* fix wrong import

* fix CI

* fix X-Pack firefox smoke test

* remove urlUtils.parseUrlQuery

* remove url.stringifyUrlQuery

* use url.encodeQuery

* Record<string, any> -> ParsedQuery

* Update src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.tsx

Co-Authored-By: Luke Elmers <lukeelmers@gmail.com>

* add more tests for APM

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>

# Conflicts:
#	x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.