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: Make sure URL's are in correct shape #306

Merged
merged 1 commit into from
Nov 30, 2023
Merged
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
25 changes: 12 additions & 13 deletions src/backend/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,34 +112,33 @@ def post_process(key: str, value: str | None) -> str:
return url


def get_ui_path() -> str:
def get_ui_path(default: str = "/") -> str:
"""
Obtain the path under which the UI should be served, e.g. `/ui`.
"""

def post_process(key: str, value: str | None) -> str:
if value is None or value == "/":
return "/"
return default
# TODO: The path should be validated here
match = re.fullmatch(r".*", value)
if match is None:
logger.error(
f"The value of the environment variable {key} is malformed: {value}" # noqa: E501
)
raise EnvironMalformedError(key)
return value if value.startswith("/") else f"/{value}"
return f"/{value.strip('/')}"

path = get_environ_str("S3GW_UI_PATH", cb=post_process)
return path
return get_environ_str("S3GW_UI_PATH", cb=post_process)


def get_api_path(ui_path: str) -> str:
"""
Obtain the path under which the API for the UI should be served from the
path of the UI itself. E.g. when the UI path is `/ui`, this will be
`/ui/api`
"""
return f"{ui_path.rstrip('/')}/api"
def get_api_path(default: str = "/api") -> str:
def post_process(_: str, value: str | None) -> str:
if value is None or value == "":
value = default
return f"/{value.strip('/')}"

return get_environ_str("S3GW_API_PATH", cb=post_process)


class Config:
Expand All @@ -164,7 +163,7 @@ def __init__(self) -> None:
"S3GW_S3_PREFIX_DELIMITER", "/"
)
self._ui_path = get_ui_path()
self._api_path = get_api_path(self._ui_path)
self._api_path = get_api_path()
self._instance_id = get_environ_str("S3GW_INSTANCE_ID")

@property
Expand Down
2 changes: 1 addition & 1 deletion src/backend/tests/unit/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ def test_config_init() -> None:
ui_resp = client.get("/s3gwui")
assert ui_resp.status_code == 200
api_resp = client.get("/s3gwui/api/buckets/")
assert api_resp.status_code == 422
assert api_resp.status_code == 404


def test_config_init_failure(caplog: pytest.LogCaptureFixture) -> None:
Expand Down
39 changes: 25 additions & 14 deletions src/backend/tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
Config,
EnvironMalformedError,
S3AddressingStyle,
get_api_path,
get_environ_enum,
get_environ_str,
get_s3gw_address,
Expand Down Expand Up @@ -130,12 +131,12 @@ def test_good_ui_path_2() -> None:

def test_good_ui_path_3() -> None:
os.environ["S3GW_UI_PATH"] = "/foo-bar/baz/"
assert "/foo-bar/baz/" == get_ui_path()
assert "/foo-bar/baz" == get_ui_path()


def test_good_ui_path_4() -> None:
os.environ["S3GW_UI_PATH"] = "/foo-bar/foo-bar/"
assert "/foo-bar/foo-bar/" == get_ui_path()
os.environ["S3GW_UI_PATH"] = "foo-bar/foo-bar"
assert "/foo-bar/foo-bar" == get_ui_path()


def test_no_ui_path() -> None:
Expand All @@ -152,17 +153,7 @@ def test_good_ui_api_path() -> None:
os.environ["S3GW_UI_PATH"] = path
try:
cfg = Config()
assert cfg.api_path == "/s3store/api"
except Exception as e:
pytest.fail(str(e))


def test_api_path_with_trailing_slash() -> None:
path = "/s3store/"
os.environ["S3GW_UI_PATH"] = path
try:
cfg = Config()
assert cfg.api_path == "/s3store/api"
assert cfg.api_path == "/api"
except Exception as e:
pytest.fail(str(e))

Expand Down Expand Up @@ -201,3 +192,23 @@ def test_get_environ_str_2() -> None:
def test_get_environ_str_3() -> None:
os.environ.pop("S3GW_INSTANCE_ID", None)
assert "" == get_environ_str("S3GW_INSTANCE_ID")


def test_api_path_1() -> None:
os.environ["S3GW_API_PATH"] = "/bar"
assert "/bar" == get_api_path()


def test_api_path_2() -> None:
os.environ["S3GW_API_PATH"] = "foo/bar/"
assert "/foo/bar" == get_api_path()


def test_api_path_3() -> None:
os.environ.pop("S3GW_API_PATH", None)
assert "/api" == get_api_path()


def test_api_path_4() -> None:
os.environ["S3GW_API_PATH"] = ""
assert "/api" == get_api_path()
12 changes: 12 additions & 0 deletions src/frontend/src/app/shared/services/api/s3gw-api.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,18 @@ describe('S3gwApiService', () => {
expect(service.buildUrl('/foo/bar/')).toBe('http://localhost:8080/api/foo/bar/');
});

it('should build valid URL [2]', () => {
service.config.ApiPath = 'api';
// @ts-ignore
expect(service.buildUrl('/foo/bar/')).toBe('api/foo/bar/');
});

it('should build valid URL [3]', () => {
service.config.ApiPath = 'aaa/api';
// @ts-ignore
expect(service.buildUrl('xyz')).toBe('aaa/api/xyz');
});

it('should call bucket list', () => {
service.config.ApiPath = 'https://localhost:8080/api';
service.get('buckets/', { credentials: { accessKey: 'foo', secretKey: 'bar' } }).subscribe();
Expand Down
5 changes: 1 addition & 4 deletions src/frontend/src/app/shared/services/api/s3gw-api.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,7 @@ export class S3gwApiService {
}

private buildUrl(url: string): string {
return `${_.trimEnd(this.config.ApiPath, this.config.Delimiter)}/${_.trimStart(
url,
this.config.Delimiter
)}`;
return `${this.config.ApiPath}/${_.trimStart(url, '/')}`;
}

private buildHeaders(credentials: Credentials): HttpHeaders {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { HttpClient } from '@angular/common/http';
import { Injectable } from '@angular/core';
import * as _ from 'lodash';
import { EMPTY, Observable } from 'rxjs';
import { catchError, tap } from 'rxjs/operators';

Expand All @@ -18,7 +19,7 @@ export type AppMainConfig = {
export class AppMainConfigService {
private _config: AppMainConfig = {
/* eslint-disable @typescript-eslint/naming-convention */
ApiPath: 'api/',
ApiPath: 'api',
Delimiter: '/',
Endpoint: '',
InstanceId: ''
Expand Down Expand Up @@ -49,6 +50,9 @@ export class AppMainConfigService {
return EMPTY;
}),
tap((config: AppMainConfig) => {
// Make sure the `ApiPath` is in a valid form, so it can be
// used without worries and adjustments.
config.ApiPath = _.trim(config.ApiPath, '/');
// eslint-disable-next-line no-underscore-dangle
this._config = config;
})
Expand Down
7 changes: 6 additions & 1 deletion src/s3gw_ui_backend.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import sys
from contextlib import asynccontextmanager
from typing import Any, AsyncGenerator, List
from urllib.parse import urljoin

import uvicorn
from fastapi import FastAPI, Response
Expand Down Expand Up @@ -120,7 +121,11 @@ def s3gw_factory(static_dir: str | None = None) -> FastAPI:
s3gw_api.include_router(objects.router)
s3gw_api.include_router(config.router)

s3gw_app.mount(s3gw_api.state.config.api_path, s3gw_api, name="api")
s3gw_app.mount(
urljoin(s3gw_api.state.config.ui_path, s3gw_api.state.config.api_path),
s3gw_api,
name="api",
)

if static_dir is not None:
# Disable caching of `index.html` on purpose so that the browser
Expand Down