-
Notifications
You must be signed in to change notification settings - Fork 31
chore: update redshift functions #235
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
Changes from all commits
20d52c5
5dad7b2
2308517
65a1bbd
1dad3f1
6360fa2
28969ba
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,9 @@ | |||||||||||||||||||||
| from fastapi.params import Depends | ||||||||||||||||||||||
| from dependency_injector.wiring import inject | ||||||||||||||||||||||
| from dependency_injector.wiring import Provide | ||||||||||||||||||||||
| from db_repo_module.models.datasource import Datasource | ||||||||||||||||||||||
| from db_repo_module.repositories.sql_alchemy_repository import SQLAlchemyRepository | ||||||||||||||||||||||
| from plugins_module.plugins_container import PluginsContainer | ||||||||||||||||||||||
| from floware.services.config_service import ConfigService | ||||||||||||||||||||||
| from user_management_module.utils.user_utils import get_current_user, check_is_admin | ||||||||||||||||||||||
| from fastapi import HTTPException | ||||||||||||||||||||||
|
|
@@ -74,6 +77,10 @@ async def get_config( | |||||||||||||||||||||
| ConfigService, | ||||||||||||||||||||||
| Depends(Provide[ApplicationContainer.config_service]), | ||||||||||||||||||||||
| ], | ||||||||||||||||||||||
| datasource_repository: Annotated[ | ||||||||||||||||||||||
| SQLAlchemyRepository[Datasource], | ||||||||||||||||||||||
| Depends(Provide[PluginsContainer.datasource_repository]), | ||||||||||||||||||||||
| ], | ||||||||||||||||||||||
| response_formatter: Annotated[ | ||||||||||||||||||||||
| ResponseFormatter, | ||||||||||||||||||||||
| Depends(Provide[CommonContainer.response_formatter]), | ||||||||||||||||||||||
|
|
@@ -84,16 +91,16 @@ async def get_config( | |||||||||||||||||||||
| such as logo, table to query, etc. | ||||||||||||||||||||||
| """ | ||||||||||||||||||||||
| url, app_config = await config_service.get_app_config() | ||||||||||||||||||||||
| if not url: | ||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||
| status_code=status.HTTP_200_OK, | ||||||||||||||||||||||
| content=response_formatter.buildSuccessResponse( | ||||||||||||||||||||||
| {'message': 'No config found'} | ||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| datasources = await datasource_repository.find() | ||||||||||||||||||||||
| datasource_ids = [str(datasource.id) for datasource in datasources] | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||
| status_code=status.HTTP_200_OK, | ||||||||||||||||||||||
| content=response_formatter.buildSuccessResponse( | ||||||||||||||||||||||
| {'app_icon': url, 'app_config': app_config} | ||||||||||||||||||||||
| { | ||||||||||||||||||||||
| 'app_icon': url, | ||||||||||||||||||||||
| 'app_config': app_config, | ||||||||||||||||||||||
| 'datasources': datasource_ids, | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
+100
to
+104
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Response shape can become inconsistent when config is missing.
💡 Proposed fix {
'app_icon': url,
- 'app_config': app_config,
+ 'app_config': app_config or {},
'datasources': datasource_ids,
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| ), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,4 +1,5 @@ | ||||||
| from .datasource_controller import datasource_router | ||||||
| from .authenticator_controller import authenticator_router | ||||||
| from .cloud_storage_controller import cloud_storage_router | ||||||
|
|
||||||
| __all__ = ['datasource_router', 'authenticator_router'] | ||||||
| __all__ = ['datasource_router', 'authenticator_router', 'cloud_storage_router'] | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sort Current ordering triggers the static-analysis warning. Proposed fix-__all__ = ['datasource_router', 'authenticator_router', 'cloud_storage_router']
+__all__ = ['authenticator_router', 'cloud_storage_router', 'datasource_router']📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.15.2)[warning] 5-5: Apply an isort-style sorting to (RUF022) 🤖 Prompt for AI Agents |
||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,102 @@ | ||||||||||||||||||||||||||
| import json | ||||||||||||||||||||||||||
| from enum import Enum | ||||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from dependency_injector.wiring import inject, Provide | ||||||||||||||||||||||||||
| from fastapi import Depends, Query, status | ||||||||||||||||||||||||||
| from fastapi.responses import JSONResponse | ||||||||||||||||||||||||||
| from fastapi.routing import APIRouter | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| from common_module.common_container import CommonContainer | ||||||||||||||||||||||||||
| from common_module.response_formatter import ResponseFormatter | ||||||||||||||||||||||||||
| from flo_cloud.cloud_storage import CloudStorageManager | ||||||||||||||||||||||||||
| from plugins_module.plugins_container import PluginsContainer | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| cloud_storage_router = APIRouter() | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| class StorageFileType(str, Enum): | ||||||||||||||||||||||||||
| json = 'json' | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @cloud_storage_router.get('/v1/storage/signed-url') | ||||||||||||||||||||||||||
| @inject | ||||||||||||||||||||||||||
| async def get_resource_presigned_url( | ||||||||||||||||||||||||||
| resource_url: str = Query(..., description='The cloud storage URL of the resource'), | ||||||||||||||||||||||||||
| expires_in: int = Query( | ||||||||||||||||||||||||||
| 300, description='Expiry time in seconds for the presigned URL' | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
|
Comment on lines
+27
to
+29
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add lower-bound validation for
Proposed fix expires_in: int = Query(
- 300, description='Expiry time in seconds for the presigned URL'
+ 300,
+ ge=1,
+ description='Expiry time in seconds for the presigned URL',
),📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| response_formatter: ResponseFormatter = Depends( | ||||||||||||||||||||||||||
| Provide[CommonContainer.response_formatter] | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| cloud_storage_manager: CloudStorageManager = Depends( | ||||||||||||||||||||||||||
| Provide[PluginsContainer.cloud_storage_manager] | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| bucket_name, key = cloud_storage_manager.get_bucket_key(resource_url) | ||||||||||||||||||||||||||
| presigned_url = cloud_storage_manager.generate_presigned_url( | ||||||||||||||||||||||||||
| bucket_name=bucket_name, | ||||||||||||||||||||||||||
| key=key, | ||||||||||||||||||||||||||
| type='GET', | ||||||||||||||||||||||||||
| expiresIn=expires_in, | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
Comment on lines
+23
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Search for auth middleware and exemptions
echo "=== Searching for auth middleware and exemptions ==="
rg -n --type=py 'RequireAuthMiddleware|exclude|exempt|public|skip_auth|@protected|@auth' | head -50
echo ""
echo "=== Searching for the endpoint and related auth code ==="
rg -n --type=py 'get_resource_presigned_url' -B5 -A10 | head -100
echo ""
echo "=== Searching for CloudStorageManager implementation ==="
rg -n --type=py 'class CloudStorageManager' -A30 | head -100
echo ""
echo "=== Searching for get_bucket_key method ==="
rg -n --type=py 'def get_bucket_key' -B3 -A15
echo ""
echo "=== Searching for authorization check patterns ==="
rg -n --type=py 'check_auth|get_current_user|verify_access|authorize|is_admin' | head -30Repository: rootflo/wavefront Length of output: 28985 🏁 Script executed: #!/bin/bash
# Search for cloud_storage_router registration and middleware setup
echo "=== Searching for cloud_storage_router registration and middleware ==="
rg -n --type=py 'cloud_storage_router|PluginsContainer' -B3 -A5 | head -80
echo ""
echo "=== Full cloud_storage_controller.py file ==="
cat wavefront/server/modules/plugins_module/plugins_module/controllers/cloud_storage_controller.py
echo ""
echo "=== Searching for current_user or authorization in plugins_module ==="
rg -n --type=py 'current_user|authorize|permission|scope' wavefront/server/modules/plugins_module/ | head -30Repository: rootflo/wavefront Length of output: 13600 Add authentication and authorization checks before generating presigned URLs. The handler accepts a caller-supplied Implement the same authorization pattern used elsewhere in the module (e.g., 🧰 Tools🪛 Ruff (0.15.2)[warning] 22-24: Do not perform function call (B008) [warning] 25-27: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||||||
| status_code=status.HTTP_200_OK, | ||||||||||||||||||||||||||
| content=response_formatter.buildSuccessResponse( | ||||||||||||||||||||||||||
| {'resource_url': presigned_url} | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||||||
| status_code=status.HTTP_400_BAD_REQUEST, | ||||||||||||||||||||||||||
| content=response_formatter.buildErrorResponse(str(e)), | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
|
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do not convert all failures into HTTP 400. Unexpected internal failures should not be reported as client input errors. Keep 400 for validation errors and let unknown exceptions propagate as 500. Proposed fix- except Exception as e:
+ except ValueError as e:
return JSONResponse(
status_code=status.HTTP_400_BAD_REQUEST,
content=response_formatter.buildErrorResponse(str(e)),
)
+ except Exception:
+ raise📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.15.2)[warning] 43-43: Do not catch blind exception: (BLE001) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @cloud_storage_router.get('/v1/storage/read') | ||||||||||||||||||||||||||
| @inject | ||||||||||||||||||||||||||
| async def read_storage_file( | ||||||||||||||||||||||||||
| resource_url: str = Query(..., description='The cloud storage URL of the resource'), | ||||||||||||||||||||||||||
| type: StorageFileType = Query(StorageFileType.json, description='File type'), | ||||||||||||||||||||||||||
| projection: Optional[str] = Query( | ||||||||||||||||||||||||||
| None, | ||||||||||||||||||||||||||
| description='Comma-separated list of top-level fields to return from the parsed data', | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| response_formatter: ResponseFormatter = Depends( | ||||||||||||||||||||||||||
| Provide[CommonContainer.response_formatter] | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| cloud_storage_manager: CloudStorageManager = Depends( | ||||||||||||||||||||||||||
| Provide[PluginsContainer.cloud_storage_manager] | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||
| bucket_name, key = cloud_storage_manager.get_bucket_key(resource_url) | ||||||||||||||||||||||||||
| file_content = cloud_storage_manager.read_file(bucket_name, key) | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| if type == StorageFileType.json: | ||||||||||||||||||||||||||
| data = json.loads(file_content) | ||||||||||||||||||||||||||
| if projection: | ||||||||||||||||||||||||||
| fields = {f.strip() for f in projection.split(',') if f.strip()} | ||||||||||||||||||||||||||
| if isinstance(data, dict): | ||||||||||||||||||||||||||
| data = {k: v for k, v in data.items() if k in fields} | ||||||||||||||||||||||||||
| elif isinstance(data, list): | ||||||||||||||||||||||||||
| data = [ | ||||||||||||||||||||||||||
| {k: v for k, v in item.items() if k in fields} | ||||||||||||||||||||||||||
| if isinstance(item, dict) | ||||||||||||||||||||||||||
| else item | ||||||||||||||||||||||||||
| for item in data | ||||||||||||||||||||||||||
| ] | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||||||
| status_code=status.HTTP_200_OK, | ||||||||||||||||||||||||||
| content=response_formatter.buildSuccessResponse({'data': data}), | ||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
| except (json.JSONDecodeError, ValueError) as e: | ||||||||||||||||||||||||||
| return JSONResponse( | ||||||||||||||||||||||||||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||||||||||||||||||||||||||
| content=response_formatter.buildErrorResponse( | ||||||||||||||||||||||||||
| f'Failed to parse file as {type.value}: {str(e)}' | ||||||||||||||||||||||||||
| ), | ||||||||||||||||||||||||||
|
Comment on lines
+96
to
+101
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Separate URL/key validation errors from JSON parse errors.
Proposed fix- except (json.JSONDecodeError, ValueError) as e:
+ except ValueError as e:
+ return JSONResponse(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ content=response_formatter.buildErrorResponse(str(e)),
+ )
+ except json.JSONDecodeError as e:
return JSONResponse(
status_code=status.HTTP_422_UNPROCESSABLE_ENTITY,
content=response_formatter.buildErrorResponse(
f'Failed to parse file as {type.value}: {str(e)}'
),
)🧰 Tools🪛 Ruff (0.15.2)[warning] 100-100: Use explicit conversion flag Replace with conversion flag (RUF010) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find()default limit silently truncates datasource IDs.Line 94 calls
datasource_repository.find()withoutlimit;SQLAlchemyRepository.find()defaults to 100, so the response can omit datasources once count exceeds 100 (seewavefront/server/modules/db_repo_module/db_repo_module/repositories/sql_alchemy_repository.py, Line 75).This makes the new
datasourcesfield incomplete under normal growth.💡 Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents