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

[Workspace] Add base path when parsing url in http service #6233

Merged
merged 7 commits into from
Mar 22, 2024
Merged
Show file tree
Hide file tree
Changes from 5 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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [BUG][MD]Expose picker using function in data source management plugin setup([#6030](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6030))
- [BUG][Multiple Datasource] Fix data source filter bug and add tests ([#6152](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6152))
- [BUG][Multiple Datasource] Fix obsolete snapshots for test within data source management plugin ([#6185](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6185))
- [Workspace] Add base path when parse url in http service ([#6233](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6233))

### 🚞 Infrastructure

Expand Down
7 changes: 4 additions & 3 deletions src/core/public/http/http_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,21 +83,22 @@ describe('#setup()', () => {
expect(setupResult.basePath.get()).toEqual('');
});

it('setup basePath with workspaceId provided in window.location.href', () => {
it('setup basePath with workspaceId provided in window.location.href and basePath present in injectedMetadata', () => {
const windowSpy = jest.spyOn(window, 'window', 'get');
windowSpy.mockImplementation(
() =>
({
location: {
href: 'http://localhost/w/workspaceId/app',
href: 'http://localhost/base_path/w/workspaceId/app',
},
} as any)
);
const injectedMetadata = injectedMetadataServiceMock.createSetupContract();
injectedMetadata.getBasePath.mockReturnValue('/base_path');
const fatalErrors = fatalErrorsServiceMock.createSetupContract();
const httpService = new HttpService();
const setupResult = httpService.setup({ fatalErrors, injectedMetadata });
expect(setupResult.basePath.get()).toEqual('/w/workspaceId');
expect(setupResult.basePath.get()).toEqual('/base_path/w/workspaceId');
windowSpy.mockRestore();
});
});
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/http/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export class HttpService implements CoreService<HttpSetup, HttpStart> {
public setup({ injectedMetadata, fatalErrors }: HttpDeps): HttpSetup {
const opensearchDashboardsVersion = injectedMetadata.getOpenSearchDashboardsVersion();
let clientBasePath = '';
const workspaceId = getWorkspaceIdFromUrl(window.location.href);
const workspaceId = getWorkspaceIdFromUrl(window.location.href, injectedMetadata.getBasePath());
if (workspaceId) {
clientBasePath = `${WORKSPACE_PATH_PREFIX}/${workspaceId}`;
}
Expand Down
4 changes: 2 additions & 2 deletions src/core/utils/workspace.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ import { httpServiceMock } from '../public/mocks';

describe('#getWorkspaceIdFromUrl', () => {
it('return workspace when there is a match', () => {
expect(getWorkspaceIdFromUrl('http://localhost/w/foo')).toEqual('foo');
expect(getWorkspaceIdFromUrl('http://localhost/w/foo', '')).toEqual('foo');
Copy link
Member

Choose a reason for hiding this comment

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

Q: why it need to specify '' as the second parameter explicitly?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe my question is more about why we need to change the getWorkspaceIdFromUrl function signature?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. From the implementation perspective, the basePath parameter is required when constructing the regular expression to parse the workspace id from url.
  2. The bug comes from the loose function signature, I change it to a more strict signature so that others won't step into similar bug.

});

it('return empty when there is not a match', () => {
expect(getWorkspaceIdFromUrl('http://localhost/w2/foo')).toEqual('');
expect(getWorkspaceIdFromUrl('http://localhost/w2/foo', '')).toEqual('');
});

it('return workspace when there is a match with basePath provided', () => {
Expand Down
2 changes: 1 addition & 1 deletion src/core/utils/workspace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
import { WORKSPACE_PATH_PREFIX } from './constants';
import { IBasePath } from '../public';

export const getWorkspaceIdFromUrl = (url: string, basePath?: string): string => {
export const getWorkspaceIdFromUrl = (url: string, basePath: string): string => {
const regexp = new RegExp(`^${basePath || ''}\/w\/([^\/]*)`);
const urlObject = new URL(url);
const matchedResult = urlObject.pathname.match(regexp);
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/workspace/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export class WorkspacePlugin implements Plugin<{}, {}, {}> {
}
}

private getWorkspaceIdFromURL(basePath?: string): string | null {
private getWorkspaceIdFromURL(basePath: string): string | null {
SuZhou-Joe marked this conversation as resolved.
Show resolved Hide resolved
return getWorkspaceIdFromUrl(window.location.href, basePath);
}

Expand Down
5 changes: 4 additions & 1 deletion src/plugins/workspace/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ export class WorkspacePlugin implements Plugin<WorkspacePluginSetup, WorkspacePl
* Proxy all {basePath}/w/{workspaceId}{osdPath*} paths to {basePath}{osdPath*}
*/
setupDeps.http.registerOnPreRouting(async (request, response, toolkit) => {
const workspaceId = getWorkspaceIdFromUrl(request.url.toString());
const workspaceId = getWorkspaceIdFromUrl(
request.url.toString(),
'' // No need to pass basePath here because the request.url will be rewrite by registerOnPreRouting method in `src/core/server/http/http_server.ts`
);

if (workspaceId) {
const requestUrl = new URL(request.url.toString());
Expand Down
Loading