diff --git a/fileglancer/database.py b/fileglancer/database.py index f1f885923..25a598522 100644 --- a/fileglancer/database.py +++ b/fileglancer/database.py @@ -628,6 +628,10 @@ def update_proxied_path(session: Session, if username != proxied_path.username: raise ValueError(f"Proxied path with sharing key {sharing_key} not found for user {username}") + # Merge into current session in case the object came from the cache + # (detached objects from a previous session won't persist changes on commit) + proxied_path = session.merge(proxied_path) + if new_sharing_name: proxied_path.sharing_name = new_sharing_name @@ -649,13 +653,14 @@ def update_proxied_path(session: Session, return proxied_path -def delete_proxied_path(session: Session, username: str, sharing_key: str): - """Delete a proxied path""" - session.query(ProxiedPathDB).filter_by(username=username, sharing_key=sharing_key).delete() +def delete_proxied_path(session: Session, username: str, sharing_key: str) -> int: + """Delete a proxied path. Returns the number of rows deleted.""" + deleted = session.query(ProxiedPathDB).filter_by(username=username, sharing_key=sharing_key).delete() session.commit() # Remove from cache _invalidate_sharing_key_cache(sharing_key) + return deleted def _generate_unique_neuroglancer_key(session: Session) -> str: diff --git a/fileglancer/model.py b/fileglancer/model.py index a46cf8d98..3aec92648 100644 --- a/fileglancer/model.py +++ b/fileglancer/model.py @@ -154,6 +154,13 @@ class ProxiedPath(BaseModel): default=None ) +class UpdateProxiedPathPayload(BaseModel): + """Payload for updating a proxied path""" + sharing_name: Optional[str] = None + fsp_name: Optional[str] = None + path: Optional[str] = None + + class ProxiedPathResponse(BaseModel): paths: List[ProxiedPath] = Field( description="A list of proxied paths" diff --git a/fileglancer/server.py b/fileglancer/server.py index 354f3ddfe..e822ad8cf 100644 --- a/fileglancer/server.py +++ b/fileglancer/server.py @@ -95,7 +95,7 @@ def _convert_external_bucket(db_bucket: db.ExternalBucketDB) -> ExternalBucket: def _convert_proxied_path(db_path: db.ProxiedPathDB, external_proxy_url: Optional[HttpUrl]) -> ProxiedPath: """Convert a database ProxiedPathDB model to a Pydantic ProxiedPath model""" if external_proxy_url: - url = f"{external_proxy_url}/{db_path.sharing_key}/{quote(db_path.sharing_name)}" + url = f"{external_proxy_url}/{db_path.sharing_key}/{quote(os.path.basename(db_path.path))}" else: logger.warning(f"No external proxy URL was provided, proxy links will not be available.") url = None @@ -199,29 +199,30 @@ def _get_user_context(username: str) -> UserContext: return CurrentUserContext() - def _get_file_proxy_client(sharing_key: str, sharing_name: str) -> Tuple[FileProxyClient, UserContext] | Tuple[Response, None]: + def _get_file_proxy_client(sharing_key: str, path_basename: str) -> Tuple[FileProxyClient, UserContext] | Tuple[Response, None]: with db.get_db_session(settings.db_url) as session: proxied_path = db.get_proxied_path_by_sharing_key(session, sharing_key) if not proxied_path: - return get_nosuchbucket_response(sharing_name), None + return get_nosuchbucket_response(path_basename), None # Vol-E viewer sends URLs with literal % characters (not URL-encoded) - # FastAPI automatically decodes path parameters - % chars are treated as escapes, creating a garbled sharing_name if they're present + # FastAPI automatically decodes path parameters - % chars are treated as escapes, creating a garbled path_basename if they're present # We therefore need to handle two cases: - # 1. Properly encoded requests (sharing_name matches DB value of proxied_path.sharing_name) - # 2. Vol-E's unencoded requests (unquote(proxied_path.sharing_name) matches the garbled request value) - if proxied_path.sharing_name != sharing_name and unquote(proxied_path.sharing_name) != sharing_name: - return get_error_response(404, "NoSuchKey", f"Sharing name mismatch for sharing key {sharing_key}", sharing_name), None + # 1. Properly encoded requests (path_basename matches os.path.basename of proxied_path.path) + # 2. Vol-E's unencoded requests (unquote of the expected basename matches the garbled request value) + expected_basename = os.path.basename(proxied_path.path) + if expected_basename != path_basename and unquote(expected_basename) != path_basename: + return get_error_response(404, "NoSuchKey", f"Path name mismatch for sharing key {sharing_key}", path_basename), None fsp = db.get_file_share_path(session, proxied_path.fsp_name) if not fsp: - return get_error_response(400, "InvalidArgument", f"File share path {proxied_path.fsp_name} not found", sharing_name), None + return get_error_response(400, "InvalidArgument", f"File share path {proxied_path.fsp_name} not found", path_basename), None # Expand ~ to user's home directory before constructing the mount path expanded_mount_path = os.path.expanduser(fsp.mount_path) mount_path = f"{expanded_mount_path}/{proxied_path.path}" # Use 256KB buffer for better performance on network filesystems - return FileProxyClient(proxy_kwargs={'target_name': sharing_name}, path=mount_path, buffer_size=256*1024), _get_user_context(proxied_path.username) + return FileProxyClient(proxy_kwargs={'target_name': os.path.basename(proxied_path.path)}, path=mount_path, buffer_size=256*1024), _get_user_context(proxied_path.username) @asynccontextmanager @@ -849,9 +850,13 @@ async def delete_neuroglancer_short_link(short_key: str = Path(..., description= description="Create a new proxied path") async def create_proxied_path(fsp_name: str = Query(..., description="The name of the file share path that this proxied path is associated with"), path: str = Query(..., description="The path relative to the file share path mount point"), + sharing_name: Optional[str] = Query(default=None, description="Display name for the data link"), username: str = Depends(get_current_user)): - sharing_name = os.path.basename(path) + if sharing_name: + sharing_name = sharing_name.strip() + if not sharing_name: + sharing_name = os.path.basename(path) logger.info(f"Creating proxied path for {username} with sharing name {sharing_name} and fsp_name {fsp_name} and path {path}") with db.get_db_session(settings.db_url) as session: with _get_user_context(username): # Necessary to validate the user can access the proxied path @@ -891,14 +896,17 @@ async def get_proxied_path(sharing_key: str = Path(..., description="The sharing @app.put("/api/proxied-path/{sharing_key}", description="Update a proxied path by sharing key") async def update_proxied_path(sharing_key: str = Path(..., description="The sharing key of the proxied path"), - fsp_name: Optional[str] = Query(default=None, description="The name of the file share path that this proxied path is associated with"), - path: Optional[str] = Query(default=None, description="The path relative to the file share path mount point"), - sharing_name: Optional[str] = Query(default=None, description="The sharing path of the proxied path"), + payload: UpdateProxiedPathPayload = Body(...), username: str = Depends(get_current_user)): + # Strip sharing_name and reject whitespace-only values + if payload.sharing_name is not None: + payload.sharing_name = payload.sharing_name.strip() + if not payload.sharing_name: + raise HTTPException(status_code=400, detail="sharing_name cannot be empty") with db.get_db_session(settings.db_url) as session: with _get_user_context(username): # Necessary to validate the user can access the proxied path try: - updated = db.update_proxied_path(session, username, sharing_key, new_path=path, new_sharing_name=sharing_name, new_fsp_name=fsp_name) + updated = db.update_proxied_path(session, username, sharing_key, new_path=payload.path, new_sharing_name=payload.sharing_name, new_fsp_name=payload.fsp_name) return _convert_proxied_path(updated, settings.external_proxy_url) except ValueError as e: logger.error(f"Error updating proxied path: {e}") @@ -970,11 +978,11 @@ async def get_neuroglancer_short_links(request: Request, return NeuroglancerShortLinkResponse(links=links) - @app.get("/files/{sharing_key}/{sharing_name}") - @app.get("/files/{sharing_key}/{sharing_name}/{path:path}") + @app.get("/files/{sharing_key}/{path_basename}") + @app.get("/files/{sharing_key}/{path_basename}/{path:path}") async def target_dispatcher(request: Request, sharing_key: str, - sharing_name: str, + path_basename: str, path: str | None = '', list_type: Optional[int] = Query(None, alias="list-type"), continuation_token: Optional[str] = Query(None, alias="continuation-token"), @@ -988,7 +996,7 @@ async def target_dispatcher(request: Request, if 'acl' in request.query_params: return get_read_access_acl() - client, ctx = _get_file_proxy_client(sharing_key, sharing_name) + client, ctx = _get_file_proxy_client(sharing_key, path_basename) if isinstance(client, Response): return client @@ -1015,10 +1023,10 @@ async def target_dispatcher(request: Request, return handle - @app.head("/files/{sharing_key}/{sharing_name}/{path:path}") - async def head_object(sharing_key: str, sharing_name: str, path: str): + @app.head("/files/{sharing_key}/{path_basename}/{path:path}") + async def head_object(sharing_key: str, path_basename: str, path: str): try: - client, ctx = _get_file_proxy_client(sharing_key, sharing_name) + client, ctx = _get_file_proxy_client(sharing_key, path_basename) if isinstance(client, Response): return client with ctx: diff --git a/frontend/src/__tests__/componentTests/DataLink.test.tsx b/frontend/src/__tests__/componentTests/DataLink.test.tsx index 5b70bb8ce..9e3c4ea65 100644 --- a/frontend/src/__tests__/componentTests/DataLink.test.tsx +++ b/frontend/src/__tests__/componentTests/DataLink.test.tsx @@ -50,8 +50,40 @@ describe('Data Link dialog', () => { }); }); + it('shows the display name input pre-populated with the path basename', async () => { + await waitFor(() => { + expect(screen.getByDisplayValue('my_zarr')).toBeInTheDocument(); + }); + const nameInput = screen.getByDisplayValue('my_zarr'); + expect(nameInput).toHaveAttribute('type', 'text'); + }); + it('calls toast.success for an ok HTTP response', async () => { const user = userEvent.setup(); + // Wait for the sharing name input to be populated from async query + await waitFor(() => { + expect(screen.getByDisplayValue('my_zarr')).toBeInTheDocument(); + }); + await user.click(screen.getByText('Create Data Link')); + await waitFor(() => { + expect(toast.success).toHaveBeenCalledWith( + 'Data link created successfully' + ); + }); + }); + + it('allows the user to set a custom sharing name', async () => { + const user = userEvent.setup(); + await waitFor(() => { + expect(screen.getByDisplayValue('my_zarr')).toBeInTheDocument(); + }); + const nameInput = screen.getByDisplayValue('my_zarr'); + + await user.clear(nameInput); + await user.type(nameInput, 'My Custom Name'); + + expect(nameInput).toHaveValue('My Custom Name'); + await user.click(screen.getByText('Create Data Link')); await waitFor(() => { expect(toast.success).toHaveBeenCalledWith( @@ -60,6 +92,23 @@ describe('Data Link dialog', () => { }); }); + it('shows validation error when sharing name is empty', async () => { + const user = userEvent.setup(); + await waitFor(() => { + expect(screen.getByDisplayValue('my_zarr')).toBeInTheDocument(); + }); + const nameInput = screen.getByDisplayValue('my_zarr'); + + await user.clear(nameInput); + + await user.click(screen.getByText('Create Data Link')); + await waitFor(() => { + expect(screen.getByText('Nickname cannot be empty')).toBeInTheDocument(); + }); + // toast.success should NOT have been called + expect(toast.success).not.toHaveBeenCalled(); + }); + it('calls toast.error for a bad HTTP response', async () => { // Override the mock for this specific test to simulate an error const { server } = await import('@/__tests__/mocks/node'); @@ -75,6 +124,10 @@ describe('Data Link dialog', () => { ); const user = userEvent.setup(); + // Wait for the sharing name input to be populated from async query + await waitFor(() => { + expect(screen.getByDisplayValue('my_zarr')).toBeInTheDocument(); + }); await user.click(screen.getByText('Create Data Link')); await waitFor(() => { expect(toast.error).toHaveBeenCalledWith( diff --git a/frontend/src/__tests__/mocks/handlers.ts b/frontend/src/__tests__/mocks/handlers.ts index e61b2d9b3..44dc5b53b 100644 --- a/frontend/src/__tests__/mocks/handlers.ts +++ b/frontend/src/__tests__/mocks/handlers.ts @@ -18,13 +18,31 @@ export const handlers = [ return HttpResponse.json({ paths: [] }, { status: 200 }); }), - http.post('/api/proxied-path', () => { + http.post('/api/proxied-path', ({ request }) => { + const url = new URL(request.url); + const sharingName = url.searchParams.get('sharing_name'); + const path = url.searchParams.get('path') ?? '/test/path'; + const pathBasename = path.split('/').pop() ?? path; return HttpResponse.json({ username: 'testuser', sharing_key: 'testkey', - sharing_name: 'testshare', - path: '/test/path', - fsp_name: 'test_fsp', + sharing_name: sharingName ?? pathBasename, + path: path, + fsp_name: url.searchParams.get('fsp_name') ?? 'test_fsp', + created_at: '2025-07-08T15:56:42.588942', + updated_at: '2025-07-08T15:56:42.588942', + url: `http://127.0.0.1:7878/files/testkey/${pathBasename}` + }); + }), + + http.put('/api/proxied-path/:sharingKey', async ({ params, request }) => { + const body = (await request.json()) as Record; + return HttpResponse.json({ + username: 'testuser', + sharing_key: params.sharingKey, + sharing_name: body.sharing_name ?? 'testshare', + path: body.path ?? '/test/path', + fsp_name: body.fsp_name ?? 'test_fsp', created_at: '2025-07-08T15:56:42.588942', updated_at: '2025-07-08T15:56:42.588942', url: 'http://127.0.0.1:7878/files/testkey/test/path' diff --git a/frontend/src/components/ui/BrowsePage/N5Preview.tsx b/frontend/src/components/ui/BrowsePage/N5Preview.tsx index 17a289f95..cc91f78b5 100644 --- a/frontend/src/components/ui/BrowsePage/N5Preview.tsx +++ b/frontend/src/components/ui/BrowsePage/N5Preview.tsx @@ -75,6 +75,7 @@ export default function N5Preview({ action="create" onCancel={handleDialogCancel} onConfirm={handleDialogConfirm} + path={path} setPendingToolKey={setPendingToolKey} setShowDataLinkDialog={setShowDataLinkDialog} showDataLinkDialog={showDataLinkDialog} diff --git a/frontend/src/components/ui/BrowsePage/ZarrPreview.tsx b/frontend/src/components/ui/BrowsePage/ZarrPreview.tsx index eac0a69fb..f2915be09 100644 --- a/frontend/src/components/ui/BrowsePage/ZarrPreview.tsx +++ b/frontend/src/components/ui/BrowsePage/ZarrPreview.tsx @@ -112,6 +112,7 @@ export default function ZarrPreview({ action="create" onCancel={handleDialogCancel} onConfirm={handleDialogConfirm} + path={path} setPendingToolKey={setPendingToolKey} setShowDataLinkDialog={setShowDataLinkDialog} showDataLinkDialog={showDataLinkDialog} diff --git a/frontend/src/components/ui/Dialogs/DataLink.tsx b/frontend/src/components/ui/Dialogs/DataLink.tsx index d11bbe27d..ee5775fca 100644 --- a/frontend/src/components/ui/Dialogs/DataLink.tsx +++ b/frontend/src/components/ui/Dialogs/DataLink.tsx @@ -1,7 +1,7 @@ /* eslint-disable react/destructuring-assignment */ // Props are used for TypeScript type narrowing purposes and cannot be destructured at the beginning -import { useState, SetStateAction } from 'react'; +import { useState, useEffect, useRef, SetStateAction } from 'react'; import type { ReactNode, Dispatch } from 'react'; import { Button, Typography } from '@material-tailwind/react'; @@ -25,7 +25,8 @@ interface CommonDataLinkDialogProps { interface CreateLinkFromToolsProps extends CommonDataLinkDialogProps { tools: true; action: 'create'; - onConfirm: () => Promise; + path: string; + onConfirm: (sharingName?: string) => Promise; onCancel: () => void; setPendingToolKey: Dispatch>; } @@ -33,7 +34,7 @@ interface CreateLinkFromToolsProps extends CommonDataLinkDialogProps { interface CreateLinkNotFromToolsProps extends CommonDataLinkDialogProps { tools: false; action: 'create'; - onConfirm: () => Promise; + onConfirm: (sharingName?: string) => Promise; onCancel: () => void; } @@ -50,16 +51,18 @@ type DataLinkDialogProps = | DeleteLinkDialogProps; function CreateLinkBtn({ - onConfirm + onConfirm, + sharingName }: { - readonly onConfirm: () => Promise; + readonly onConfirm: (sharingName?: string) => Promise; + readonly sharingName?: string; }) { return (