Skip to content

Commit

Permalink
[Refactor] Replace url parse format resolve with whatwg url (opensear…
Browse files Browse the repository at this point in the history
…ch-project#2910)

Replace the url.parse/url.format/url.resolve function in the code base with whatwg-url.
The main change is to change the url.parse to new URL(), and change url.format to [URL object].toString and change the url.resolve to new URL() toString().
Another major change is to replace the url parts in test config with URL object for transmission. 

Partially completes opensearch-project#1561

Signed-off-by: Lin Wang <[email protected]>
Co-authored-by: Josh Romero <[email protected]>
  • Loading branch information
2 people authored and Peter Fitzgibbons committed Mar 24, 2023
1 parent 79a1052 commit bd51e23
Show file tree
Hide file tree
Showing 49 changed files with 240 additions and 289 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- Temporary workaround for task-kill exceptions on Windows when it is passed a pid for a process that is already dead ([#2842](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2842))
- [Vis Builder] Fix empty workspace animation does not work in firefox ([#2853](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2853))
- Bumped `del` version to fix MacOS race condition ([#2847](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2873))
- [Chore] Update deprecated url methods (url.parse(), url.format()) ([#1561](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/1561))
- [Build] Fixed "Last Access Time" not being set by `scanCopy` on Windows ([#2964](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2964))
- [Vis Builder] Add global data persistence for vis builder #2896 ([#2896](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2896))
- Update `leaflet-vega` and fix its usage ([#3005](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/3005))
Expand Down
14 changes: 4 additions & 10 deletions examples/url_generators_examples/public/url_generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import url from 'url';
import { UrlGeneratorState, UrlGeneratorsDefinition } from '../../../src/plugins/share/public';

/**
Expand All @@ -53,16 +52,11 @@ export const createHelloPageLinkGenerator = (
createUrl: async (state) => {
const startServices = await getStartServices();
const appBasePath = startServices.appBasePath;
const parsedUrl = url.parse(window.location.href);

return url.format({
protocol: parsedUrl.protocol,
host: parsedUrl.host,
pathname: `${appBasePath}/hello`,
query: {
...state,
},
});
const url = new URL(`${appBasePath}/hello`, window.location.origin);
url.search = new URLSearchParams({ ...state }).toString();

return url.toString();
},
});

Expand Down
7 changes: 3 additions & 4 deletions packages/osd-dev-utils/src/osd_client/osd_client_requester.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import Url from 'url';
import Https from 'https';
import Axios, { AxiosResponse } from 'axios';

Expand Down Expand Up @@ -93,7 +92,7 @@ export class OsdClientRequester {
constructor(private readonly log: ToolingLog, options: Options) {
this.url = options.url;
this.httpsAgent =
Url.parse(options.url).protocol === 'https:'
new URL(options.url).protocol === 'https:'
? new Https.Agent({
ca: options.certificateAuthorities,
})
Expand All @@ -105,11 +104,11 @@ export class OsdClientRequester {
}

public resolveUrl(relativeUrl: string = '/') {
return Url.resolve(this.pickUrl(), relativeUrl);
return new URL(relativeUrl, this.pickUrl()).toString();
}

async request<T>(options: ReqOptions): Promise<AxiosResponse<T>> {
const url = Url.resolve(this.pickUrl(), options.path);
const url = new URL(options.path, this.pickUrl()).toString();
const description = options.description || `${options.method} ${url}`;
let attempt = 0;
const maxAttempts = options.retries ?? DEFAULT_MAX_ATTEMPTS;
Expand Down
5 changes: 2 additions & 3 deletions packages/osd-opensearch-archiver/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@
*************************************************************/

import Path from 'path';
import Url from 'url';
import readline from 'readline';

import { RunWithCommands, createFlagError } from '@osd/dev-utils';
Expand Down Expand Up @@ -72,7 +71,7 @@ export function runCli() {
throw createFlagError('--opensearch-url must be a string');
}
if (!opensearchUrl && config) {
opensearchUrl = Url.format(config.get('servers.opensearch'));
opensearchUrl = config.get('servers.opensearch.serverUrl');
}
if (!opensearchUrl) {
throw createFlagError('--opensearch-url or --config must be defined');
Expand All @@ -83,7 +82,7 @@ export function runCli() {
throw createFlagError('--opensearch-dashboards-url must be a string');
}
if (!opensearchDashboardsUrl && config) {
opensearchDashboardsUrl = Url.format(config.get('servers.opensearchDashboards'));
opensearchDashboardsUrl = config.get('servers.opensearchDashboards.serverUrl') as string;
}
if (!opensearchDashboardsUrl) {
throw createFlagError('---url or --config must be defined');
Expand Down
10 changes: 4 additions & 6 deletions packages/osd-test/src/failed_tests_reporter/github_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
* under the License.
*/

import Url from 'url';

import Axios, { AxiosRequestConfig, AxiosInstance } from 'axios';
import parseLinkHeader from 'parse-link-header';
import { ToolingLog, isAxiosResponseError, isAxiosRequestError } from '@osd/dev-utils';
Expand Down Expand Up @@ -98,7 +96,7 @@ export class GithubApi {
nextRequest: {
safeForDryRun: true,
method: 'GET',
url: Url.resolve(BASE_URL, 'issues'),
url: new URL('issues', BASE_URL).toString(),
params: {
state: 'all',
per_page: '100',
Expand Down Expand Up @@ -158,7 +156,7 @@ export class GithubApi {
await this.request(
{
method: 'PATCH',
url: Url.resolve(BASE_URL, `issues/${encodeURIComponent(issueNumber)}`),
url: new URL(`issues/${encodeURIComponent(issueNumber)}`, BASE_URL).toString(),
data: {
state: 'open', // Reopen issue if it was closed.
body: newBody,
Expand All @@ -172,7 +170,7 @@ export class GithubApi {
await this.request(
{
method: 'POST',
url: Url.resolve(BASE_URL, `issues/${encodeURIComponent(issueNumber)}/comments`),
url: new URL(`issues/${encodeURIComponent(issueNumber)}/comments`, BASE_URL).toString(),
data: {
body: commentBody,
},
Expand All @@ -185,7 +183,7 @@ export class GithubApi {
const resp = await this.request<GithubIssueMini>(
{
method: 'POST',
url: Url.resolve(BASE_URL, 'issues'),
url: new URL('issues', BASE_URL).toString(),
data: {
title,
body,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,10 @@ export class Config {
if (typeof v === 'function') {
return v;
}

if (v instanceof URL) {
return new URL(v.toString());
}
});
}

Expand All @@ -134,6 +138,10 @@ export class Config {
if (typeof v === 'function') {
return v;
}

if (v instanceof URL) {
return new URL(v.toString());
}
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ const urlPartsSchema = () =>
pathname: Joi.string().regex(/^\//, 'start with a /'),
hash: Joi.string().regex(/^\//, 'start with a /'),
certificateAuthorities: Joi.array().items(Joi.binary()).optional(),
fullURL: Joi.object().type(URL),
serverUrl: Joi.string(),
})
.default();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import Url from 'url';
import execa from 'execa';
import * as Rx from 'rxjs';
import { filter, take, map } from 'rxjs/operators';
Expand All @@ -54,11 +53,7 @@ export class DockerServersService {
this.servers = Object.entries(configs).map(([name, config]) => ({
...config,
name,
url: Url.format({
protocol: 'http:',
hostname: 'localhost',
port: config.port,
}),
url: new URL(`http://localhost:${config.port}`).toString(),
}));

this.lifecycle.beforeTests.add(async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
*/

import { resolve } from 'path';
import { format } from 'url';
import { get, toPath } from 'lodash';
import { Cluster } from '@osd/opensearch';
import { CI_PARALLEL_PROCESS_PREFIX } from '../ci_parallel_process_prefix';
Expand Down Expand Up @@ -135,10 +134,10 @@ export function createLegacyOpenSearchTestCluster(options = {}) {
}

getUrl() {
const parts = opensearchTestConfig.getUrlParts();
parts.port = port;
const url = new URL(opensearchTestConfig.getUrlParts().fullURL);
url.port = port;

return format(parts);
return url.toString().slice(0, -1);
}
})();
}
Expand Down
26 changes: 17 additions & 9 deletions packages/osd-test/src/legacy_opensearch/opensearch_test_config.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import url, { format as formatUrl } from 'url';
import pkg from '../../../../package.json';
import { adminTestUser } from '../osd';

Expand All @@ -42,7 +41,7 @@ export const opensearchTestConfig = new (class OpenSearchTestConfig {
}

getUrl() {
return formatUrl(this.getUrlParts());
return this.getUrlParts().serverUrl;
}

getBuildFrom() {
Expand All @@ -56,30 +55,39 @@ export const opensearchTestConfig = new (class OpenSearchTestConfig {
getUrlParts() {
// Allow setting one complete TEST_OPENSEARCH_URL for opensearch like https://opensearch:[email protected]:9200
if (process.env.TEST_OPENSEARCH_URL) {
const testOpenSearchUrl = url.parse(process.env.TEST_OPENSEARCH_URL);
const testOpenSearchUrl = new URL('', process.env.TEST_OPENSEARCH_URL);
testOpenSearchUrl.pathname = '';
return {
// have to remove the ":" off protocol
protocol: testOpenSearchUrl.protocol.slice(0, -1),
hostname: testOpenSearchUrl.hostname,
port: parseInt(testOpenSearchUrl.port, 10),
username: testOpenSearchUrl.auth.split(':')[0],
password: testOpenSearchUrl.auth.split(':')[1],
auth: testOpenSearchUrl.auth,
username: testOpenSearchUrl.username,
password: testOpenSearchUrl.password,
auth: `${testOpenSearchUrl.username}:${testOpenSearchUrl.password}`,
fullURL: testOpenSearchUrl,
serverUrl: testOpenSearchUrl.toString().slice(0, -1),
};
}

const username = process.env.TEST_OPENSEARCH_USERNAME || adminTestUser.username;
const password = process.env.TEST_OPENSEARCH_PASSWORD || adminTestUser.password;
const protocol = process.env.TEST_OPENSEARCH_PROTOCOL || 'http';
const hostname = process.env.TEST_OPENSEARCH_HOSTNAME || 'localhost';
const port = parseInt(process.env.TEST_OPENSEARCH_PORT, 10) || 9220;
const fullURL = new URL('', `${protocol}://${username}:${password}@${hostname}:${port}`);

return {
// Allow setting any individual component(s) of the URL,
// or use default values (username and password from ../osd/users.js)
protocol: process.env.TEST_OPENSEARCH_PROTOCOL || 'http',
hostname: process.env.TEST_OPENSEARCH_HOSTNAME || 'localhost',
port: parseInt(process.env.TEST_OPENSEARCH_PORT, 10) || 9220,
protocol,
hostname,
port,
auth: `${username}:${password}`,
username: username,
password: password,
fullURL,
serverUrl: fullURL.toString().slice(0, -1),
};
}
})();
34 changes: 24 additions & 10 deletions packages/osd-test/src/osd/osd_test_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@
* under the License.
*/

import url from 'url';
import { opensearchDashboardsTestUser } from './users';

interface UrlParts {
Expand All @@ -38,6 +37,8 @@ interface UrlParts {
auth?: string;
username?: string;
password?: string;
fullURL: URL;
serverUrl: string;
}

export const osdTestConfig = new (class OsdTestConfig {
Expand All @@ -48,32 +49,45 @@ export const osdTestConfig = new (class OsdTestConfig {
getUrlParts(): UrlParts {
// allow setting one complete TEST_OPENSEARCH_DASHBOARDS_URL for opensearch like https://opensearch:[email protected]:9200
if (process.env.TEST_OPENSEARCH_DASHBOARDS_URL) {
const testOpenSearchDashboardsUrl = url.parse(process.env.TEST_OPENSEARCH_DASHBOARDS_URL);
const testOpenSearchDashboardsUrl = new URL('', process.env.TEST_OPENSEARCH_DASHBOARDS_URL);
testOpenSearchDashboardsUrl.pathname = '';
return {
protocol: testOpenSearchDashboardsUrl.protocol?.slice(0, -1),
hostname: testOpenSearchDashboardsUrl.hostname ?? undefined,
port: testOpenSearchDashboardsUrl.port
? parseInt(testOpenSearchDashboardsUrl.port, 10)
: undefined,
auth: testOpenSearchDashboardsUrl.auth ?? undefined,
username: testOpenSearchDashboardsUrl.auth?.split(':')[0],
password: testOpenSearchDashboardsUrl.auth?.split(':')[1],
auth:
testOpenSearchDashboardsUrl.username && testOpenSearchDashboardsUrl.password
? `${testOpenSearchDashboardsUrl.username}:${testOpenSearchDashboardsUrl.password}`
: undefined,
username: testOpenSearchDashboardsUrl.username ?? undefined,
password: testOpenSearchDashboardsUrl.password ?? undefined,
fullURL: testOpenSearchDashboardsUrl,
serverUrl: testOpenSearchDashboardsUrl.toString().slice(0, -1),
};
}

const username =
process.env.TEST_OPENSEARCH_DASHBOARDS_USERNAME || opensearchDashboardsTestUser.username;
const password =
process.env.TEST_OPENSEARCH_DASHBOARDS_PASSWORD || opensearchDashboardsTestUser.password;
const protocol = process.env.TEST_OPENSEARCH_DASHBOARDS_PROTOCOL || 'http';
const hostname = process.env.TEST_OPENSEARCH_DASHBOARDS_HOSTNAME || 'localhost';
const port = process.env.TEST_OPENSEARCH_DASHBOARDS_PORT
? parseInt(process.env.TEST_OPENSEARCH_DASHBOARDS_PORT, 10)
: 5620;
const fullURL = new URL(`${protocol}://${username}:${password}@${hostname}:${port}`);

return {
protocol: process.env.TEST_OPENSEARCH_DASHBOARDS_PROTOCOL || 'http',
hostname: process.env.TEST_OPENSEARCH_DASHBOARDS_HOSTNAME || 'localhost',
port: process.env.TEST_OPENSEARCH_DASHBOARDS_PORT
? parseInt(process.env.TEST_OPENSEARCH_DASHBOARDS_PORT, 10)
: 5620,
protocol,
hostname,
port,
auth: `${username}:${password}`,
username,
password,
fullURL,
serverUrl: fullURL.toString().slice(0, -1),
};
}
})();
3 changes: 1 addition & 2 deletions src/cli/serve/serve.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import { set as lodashSet } from '@elastic/safer-lodash-set';
import _ from 'lodash';
import { statSync } from 'fs';
import { resolve } from 'path';
import url from 'url';

import { getConfigPath } from '@osd/utils';
import { IS_OPENSEARCH_DASHBOARDS_DISTRIBUTABLE } from '../../legacy/utils';
Expand Down Expand Up @@ -110,7 +109,7 @@ function applyConfigOverrides(rawConfig, opts, extraCliOptions) {
const opensearchHosts = (
(customOpenSearchHosts.length > 0 && customOpenSearchHosts) || ['https://localhost:9200']
).map((hostUrl) => {
const parsedUrl = url.parse(hostUrl);
const parsedUrl = new URL('', hostUrl);
if (parsedUrl.hostname !== 'localhost') {
throw new Error(
`Hostname "${parsedUrl.hostname}" can't be used with --ssl. Must be "localhost" to work with certificates.`
Expand Down
11 changes: 7 additions & 4 deletions src/cli_plugin/install/download.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
* under the License.
*/

import { parse } from 'url';

import { UnsupportedProtocolError } from '../lib/errors';
import { downloadHttpFile } from './downloaders/http';
import { downloadLocalFile } from './downloaders/file';
Expand All @@ -56,14 +54,19 @@ export function _checkFilePathDeprecation(sourceUrl, logger) {
}

export function _downloadSingle(settings, logger, sourceUrl) {
const urlInfo = parse(sourceUrl);
let urlInfo;
try {
urlInfo = new URL('', sourceUrl);
} catch (e) {
return Promise.reject(new UnsupportedProtocolError());
}
let downloadPromise;

if (/^file/.test(urlInfo.protocol)) {
_checkFilePathDeprecation(sourceUrl, logger);
downloadPromise = downloadLocalFile(
logger,
_getFilePath(urlInfo.path, sourceUrl),
_getFilePath(urlInfo.pathname, sourceUrl),
settings.tempArchiveFile
);
} else if (/^https?/.test(urlInfo.protocol)) {
Expand Down
Loading

0 comments on commit bd51e23

Please sign in to comment.