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 workspace id in basePath #6060

Merged
Merged
Show file tree
Hide file tree
Changes from 12 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 @@ -29,6 +29,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Multiple Datasource] Handle form values(request payload) if the selected type is available in the authentication registry ([#6049](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6049))
- [Multiple Datasource] Add Vega support to MDS by specifying a data source name in the Vega spec ([#5975](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5975))
- [Workspace] Consume workspace id in saved object client ([#6014](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6014))
- [Workspace] Add workspace id in basePath ([#6060](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6060))

- [Workspace] Add delete saved objects by workspace functionality([#6013](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/6013))

Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

40 changes: 40 additions & 0 deletions src/core/public/http/base_path.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,4 +110,44 @@ describe('BasePath', () => {
expect(new BasePath('/foo/bar', '/foo').serverBasePath).toEqual('/foo');
});
});

describe('clientBasePath', () => {
it('get path with workspace', () => {
SuZhou-Joe marked this conversation as resolved.
Show resolved Hide resolved
expect(new BasePath('/foo/bar', '/foo/bar', '/workspace').get()).toEqual(
'/foo/bar/workspace'
);
});

it('getBasePath with workspace provided', () => {
expect(new BasePath('/foo/bar', '/foo/bar', '/workspace').getBasePath()).toEqual('/foo/bar');
});

it('prepend with workspace provided', () => {
expect(new BasePath('/foo/bar', '/foo/bar', '/workspace').prepend('/prepend')).toEqual(
'/foo/bar/workspace/prepend'
);
});

it('prepend with workspace provided but calls without workspace', () => {
expect(
new BasePath('/foo/bar', '/foo/bar', '/workspace').prepend('/prepend', {
withoutWorkspace: true,
})
).toEqual('/foo/bar/prepend');
});

it('remove with workspace provided', () => {
expect(
new BasePath('/foo/bar', '/foo/bar', '/workspace').remove('/foo/bar/workspace/remove')
).toEqual('/remove');
});

it('remove with workspace provided but calls without workspace', () => {
expect(
new BasePath('/foo/bar', '/foo/bar', '/workspace').remove('/foo/bar/workspace/remove', {
withoutWorkspace: true,
})
).toEqual('/workspace/remove');
});
});
});
28 changes: 19 additions & 9 deletions src/core/public/http/base_path.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,37 +29,47 @@
*/

import { modifyUrl } from '@osd/std';
import type { PrependOptions } from './types';

export class BasePath {
constructor(
private readonly basePath: string = '',
public readonly serverBasePath: string = basePath
public readonly serverBasePath: string = basePath,
private readonly clientBasePath: string = ''
) {}

public get = () => {
return `${this.basePath}${this.clientBasePath}`;
};

public getBasePath = () => {
return this.basePath;
};

public prepend = (path: string): string => {
if (!this.basePath) return path;
public prepend = (path: string, prependOptions?: PrependOptions): string => {
const { withoutWorkspace } = prependOptions || {};
const basePath = withoutWorkspace ? this.basePath : this.get();
if (!basePath) return path;
return modifyUrl(path, (parts) => {
if (!parts.hostname && parts.pathname && parts.pathname.startsWith('/')) {
parts.pathname = `${this.basePath}${parts.pathname}`;
parts.pathname = `${basePath}${parts.pathname}`;

Check warning on line 55 in src/core/public/http/base_path.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/base_path.ts#L55

Added line #L55 was not covered by tests
}
});
};

public remove = (path: string): string => {
if (!this.basePath) {
public remove = (path: string, prependOptions?: PrependOptions): string => {
const { withoutWorkspace } = prependOptions || {};
const basePath = withoutWorkspace ? this.basePath : this.get();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can rename the "workspace" in prepend and remove function as well^^

Copy link
Member Author

Choose a reason for hiding this comment

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

I was intending to rename the withoutWorkspace to withoutClientBasePath but I was thinking in the future, the clientBasePath may consist of workspace/dataSourceId/other stuff and prependOptions will have withoutWorkspace/withoutDataSourceId/withoutClientBasePath options accordingly so I kept them.

But for now, there is no such case so it has no harm to have only one withoutClientBasePath config in prependOptions, but will add some comment here.

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks for the clarification^^

if (!basePath) {
return path;
}

if (path === this.basePath) {
if (path === basePath) {
return '/';
}

if (path.startsWith(`${this.basePath}/`)) {
return path.slice(this.basePath.length);
if (path.startsWith(`${basePath}/`)) {
return path.slice(basePath.length);

Check warning on line 72 in src/core/public/http/base_path.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/base_path.ts#L72

Added line #L72 was not covered by tests
}

return path;
Expand Down
10 changes: 5 additions & 5 deletions src/core/public/http/http_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
anonymousPaths: jest.Mocked<HttpSetup['anonymousPaths']>;
};

const createServiceMock = ({ basePath = '' } = {}): HttpSetupMock => ({
const createServiceMock = ({ basePath = '', clientBasePath = '' } = {}): HttpSetupMock => ({
fetch: jest.fn(),
get: jest.fn(),
head: jest.fn(),
Expand All @@ -48,7 +48,7 @@
patch: jest.fn(),
delete: jest.fn(),
options: jest.fn(),
basePath: new BasePath(basePath),
basePath: new BasePath(basePath, undefined, clientBasePath),
anonymousPaths: {
register: jest.fn(),
isAnonymous: jest.fn(),
Expand All @@ -58,14 +58,14 @@
intercept: jest.fn(),
});

const createMock = ({ basePath = '' } = {}) => {
const createMock = ({ basePath = '', clientBasePath = '' } = {}) => {
const mocked: jest.Mocked<PublicMethodsOf<HttpService>> = {
setup: jest.fn(),
start: jest.fn(),
stop: jest.fn(),
};
mocked.setup.mockReturnValue(createServiceMock({ basePath }));
mocked.start.mockReturnValue(createServiceMock({ basePath }));
mocked.setup.mockReturnValue(createServiceMock({ basePath, clientBasePath }));
mocked.start.mockReturnValue(createServiceMock({ basePath, clientBasePath }));

Check warning on line 68 in src/core/public/http/http_service.mock.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/http_service.mock.ts#L67-L68

Added lines #L67 - L68 were not covered by tests
return mocked;
};

Expand Down
57 changes: 57 additions & 0 deletions src/core/public/http/http_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,63 @@ describe('#setup()', () => {
// We don't verify that this Observable comes from Fetch#getLoadingCount$() to avoid complex mocking
expect(loadingServiceSetup.addLoadingCountSource).toHaveBeenCalledWith(expect.any(Observable));
});

it('setup basePath without workspaceId provided in window.location.href', () => {
const injectedMetadata = injectedMetadataServiceMock.createSetupContract();
const fatalErrors = fatalErrorsServiceMock.createSetupContract();
const httpService = new HttpService();
const setupResult = httpService.setup({ fatalErrors, injectedMetadata });
expect(setupResult.basePath.get()).toEqual('');
});

it('setup basePath with workspaceId provided in window.location.href and workspace feature enabled', () => {
const windowSpy = jest.spyOn(window, 'window', 'get');
windowSpy.mockImplementation(
() =>
({
location: {
href: 'http://localhost/w/workspaceId/app',
},
} as any)
);
const injectedMetadata = injectedMetadataServiceMock.createSetupContract();
injectedMetadata.getPlugins.mockReturnValueOnce([
{
id: 'workspace',
plugin: {
id: 'workspace',
configPath: '',
requiredPlugins: [],
optionalPlugins: [],
requiredEnginePlugins: {},
requiredBundles: [],
},
},
]);
const fatalErrors = fatalErrorsServiceMock.createSetupContract();
const httpService = new HttpService();
const setupResult = httpService.setup({ fatalErrors, injectedMetadata });
expect(setupResult.basePath.get()).toEqual('/w/workspaceId');
windowSpy.mockRestore();
});

it('setup basePath with workspaceId provided in window.location.href but workspace feature disabled', () => {
const windowSpy = jest.spyOn(window, 'window', 'get');
windowSpy.mockImplementation(
() =>
({
location: {
href: 'http://localhost/w/workspaceId/app',
},
} as any)
);
const injectedMetadata = injectedMetadataServiceMock.createSetupContract();
const fatalErrors = fatalErrorsServiceMock.createSetupContract();
const httpService = new HttpService();
const setupResult = httpService.setup({ fatalErrors, injectedMetadata });
expect(setupResult.basePath.get()).toEqual('');
windowSpy.mockRestore();
});
});

describe('#stop()', () => {
Expand Down
18 changes: 17 additions & 1 deletion src/core/public/http/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
import { LoadingCountService } from './loading_count_service';
import { Fetch } from './fetch';
import { CoreService } from '../../types';
import { getWorkspaceIdFromUrl } from '../utils';
import { WORKSPACE_PATH_PREFIX } from '../../utils/constants';

interface HttpDeps {
injectedMetadata: InjectedMetadataSetup;
Expand All @@ -50,9 +52,23 @@

public setup({ injectedMetadata, fatalErrors }: HttpDeps): HttpSetup {
const opensearchDashboardsVersion = injectedMetadata.getOpenSearchDashboardsVersion();
let clientBasePath = '';
const plugins = injectedMetadata.getPlugins();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Is there precedence for depending on a plugin in core? i dont see any use of the getPlugins API in core. Also if this is a temporary dependency, can we add a TODO here and link it to an issue where we remove it in future so that core can remain independent of plugins?

Copy link
Member Author

@SuZhou-Joe SuZhou-Joe Mar 8, 2024

Choose a reason for hiding this comment

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

Yes but actually what I want to do here is to make the workspace id logic disabled when workspace is not enabled. But it turns out that it will introduce another indirect dependency on the workspace plugin. I can revert to previous version, in which it will try to construct workspaceBasePath no matter workspace is enabled or not, and let server side to gives a 404 when workspace is disabled, do you think that would be better?

const findWorkspaceConfig = plugins.find((plugin) => plugin.id === 'workspace');

Check warning on line 57 in src/core/public/http/http_service.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/http_service.ts#L55-L57

Added lines #L55 - L57 were not covered by tests
// Only try to get workspace id from url when workspace feature is enabled
if (findWorkspaceConfig) {
const workspaceId = getWorkspaceIdFromUrl(

Check warning on line 60 in src/core/public/http/http_service.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/http_service.ts#L60

Added line #L60 was not covered by tests
window.location.href,
injectedMetadata.getBasePath()
);
if (workspaceId) {
clientBasePath = `${WORKSPACE_PATH_PREFIX}/${workspaceId}`;

Check warning on line 65 in src/core/public/http/http_service.ts

View check run for this annotation

Codecov / codecov/patch

src/core/public/http/http_service.ts#L65

Added line #L65 was not covered by tests
}
}
const basePath = new BasePath(
injectedMetadata.getBasePath(),
injectedMetadata.getServerBasePath()
injectedMetadata.getServerBasePath(),
clientBasePath
);
const fetchService = new Fetch({ basePath, opensearchDashboardsVersion });
const loadingCount = this.loadingCount.setup({ fatalErrors });
Expand Down
Loading
Loading