-
Notifications
You must be signed in to change notification settings - Fork 2
feat(management): add FastAPI routes and dependency injection (AIHCM-185) #303
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
954a84e
d296a87
5103aa6
724aa0f
cfce711
74e6ab0
634108b
5b7d0a6
800e749
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 | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -200,18 +200,24 @@ async def list_for_workspace( | |||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||
| user_id: str, | ||||||||||||||||||||||||||||||||||
| workspace_id: str, | ||||||||||||||||||||||||||||||||||
| ) -> list[KnowledgeGraph]: | ||||||||||||||||||||||||||||||||||
| """List knowledge graphs in a workspace. | ||||||||||||||||||||||||||||||||||
| *, | ||||||||||||||||||||||||||||||||||
| offset: int = 0, | ||||||||||||||||||||||||||||||||||
| limit: int = 20, | ||||||||||||||||||||||||||||||||||
| ) -> tuple[list[KnowledgeGraph], int]: | ||||||||||||||||||||||||||||||||||
| """List knowledge graphs in a workspace with pagination. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Uses read_relationships to discover KG IDs linked to the workspace, | ||||||||||||||||||||||||||||||||||
| then fetches each from the repository and filters by tenant. | ||||||||||||||||||||||||||||||||||
| Pagination is applied after filtering. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||
| user_id: The user requesting the list | ||||||||||||||||||||||||||||||||||
| workspace_id: The workspace to list KGs for | ||||||||||||||||||||||||||||||||||
| offset: Number of records to skip | ||||||||||||||||||||||||||||||||||
| limit: Maximum number of records to return | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||
| List of KnowledgeGraph aggregates | ||||||||||||||||||||||||||||||||||
| Tuple of (paginated KnowledgeGraph aggregates, total count) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||||
| UnauthorizedError: If user lacks VIEW permission on workspace | ||||||||||||||||||||||||||||||||||
|
|
@@ -250,33 +256,37 @@ async def list_for_workspace( | |||||||||||||||||||||||||||||||||
| kg_ids.append(parts[1]) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| # Fetch each KG from repo and filter by tenant | ||||||||||||||||||||||||||||||||||
| # (N+1 problem - acceptable for walking skeleton) | ||||||||||||||||||||||||||||||||||
| kgs: list[KnowledgeGraph] = [] | ||||||||||||||||||||||||||||||||||
| for kg_id in kg_ids: | ||||||||||||||||||||||||||||||||||
| kg = await self._kg_repo.get_by_id(KnowledgeGraphId(value=kg_id)) | ||||||||||||||||||||||||||||||||||
| if kg is not None and kg.tenant_id == self._scope_to_tenant: | ||||||||||||||||||||||||||||||||||
| kgs.append(kg) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| total = len(kgs) | ||||||||||||||||||||||||||||||||||
| paginated = kgs[offset : offset + limit] | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||
| self._probe.knowledge_graphs_listed( | ||||||||||||||||||||||||||||||||||
| workspace_id=workspace_id, | ||||||||||||||||||||||||||||||||||
| count=len(kgs), | ||||||||||||||||||||||||||||||||||
| count=len(paginated), | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return kgs | ||||||||||||||||||||||||||||||||||
| return paginated, total | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| async def update( | ||||||||||||||||||||||||||||||||||
| self, | ||||||||||||||||||||||||||||||||||
| user_id: str, | ||||||||||||||||||||||||||||||||||
| kg_id: str, | ||||||||||||||||||||||||||||||||||
| name: str, | ||||||||||||||||||||||||||||||||||
| description: str, | ||||||||||||||||||||||||||||||||||
| name: str | None = None, | ||||||||||||||||||||||||||||||||||
| description: str | None = None, | ||||||||||||||||||||||||||||||||||
| ) -> KnowledgeGraph: | ||||||||||||||||||||||||||||||||||
| """Update a knowledge graph's metadata. | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||
| user_id: The user performing the update | ||||||||||||||||||||||||||||||||||
| kg_id: The knowledge graph ID | ||||||||||||||||||||||||||||||||||
| name: New name | ||||||||||||||||||||||||||||||||||
| description: New description | ||||||||||||||||||||||||||||||||||
| name: Optional new name (uses existing if None) | ||||||||||||||||||||||||||||||||||
| description: Optional new description (uses existing if None) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||
| The updated KnowledgeGraph aggregate | ||||||||||||||||||||||||||||||||||
|
|
@@ -310,17 +320,26 @@ async def update( | |||||||||||||||||||||||||||||||||
| if kg.tenant_id != self._scope_to_tenant: | ||||||||||||||||||||||||||||||||||
| raise ValueError(f"Knowledge graph {kg_id} not found") | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| kg.update(name=name, description=description, updated_by=user_id) | ||||||||||||||||||||||||||||||||||
| resolved_name = name if name is not None else kg.name | ||||||||||||||||||||||||||||||||||
| resolved_description = ( | ||||||||||||||||||||||||||||||||||
| description if description is not None else kg.description | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| kg.update( | ||||||||||||||||||||||||||||||||||
| name=resolved_name, | ||||||||||||||||||||||||||||||||||
| description=resolved_description, | ||||||||||||||||||||||||||||||||||
| updated_by=user_id, | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||
| async with self._session.begin(): | ||||||||||||||||||||||||||||||||||
| await self._kg_repo.save(kg) | ||||||||||||||||||||||||||||||||||
| except IntegrityError as e: | ||||||||||||||||||||||||||||||||||
| raise DuplicateKnowledgeGraphNameError( | ||||||||||||||||||||||||||||||||||
| f"Knowledge graph '{name}' already exists in tenant" | ||||||||||||||||||||||||||||||||||
| f"Knowledge graph '{resolved_name}' already exists in tenant" | ||||||||||||||||||||||||||||||||||
| ) from e | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| self._probe.knowledge_graph_updated(kg_id=kg_id, name=name) | ||||||||||||||||||||||||||||||||||
| self._probe.knowledge_graph_updated(kg_id=kg_id, name=resolved_name) | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return kg | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
@@ -368,7 +387,9 @@ async def delete( | |||||||||||||||||||||||||||||||||
| async with self._session.begin(): | ||||||||||||||||||||||||||||||||||
| # Cascade delete data sources if repo is available | ||||||||||||||||||||||||||||||||||
| if self._ds_repo is not None: | ||||||||||||||||||||||||||||||||||
| data_sources = await self._ds_repo.find_by_knowledge_graph(kg_id) | ||||||||||||||||||||||||||||||||||
| data_sources, _ = await self._ds_repo.find_by_knowledge_graph( | ||||||||||||||||||||||||||||||||||
| kg_id, offset=0, limit=10000 | ||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||
| for ds in data_sources: | ||||||||||||||||||||||||||||||||||
| ds.mark_for_deletion(deleted_by=user_id) | ||||||||||||||||||||||||||||||||||
| await self._ds_repo.delete(ds) | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
| data_sources, _ = await self._ds_repo.find_by_knowledge_graph( | |
| kg_id, offset=0, limit=10000 | |
| ) | |
| for ds in data_sources: | |
| ds.mark_for_deletion(deleted_by=user_id) | |
| await self._ds_repo.delete(ds) | |
| page_size = 500 | |
| while True: | |
| data_sources, _ = await self._ds_repo.find_by_knowledge_graph( | |
| kg_id, offset=0, limit=page_size | |
| ) | |
| if not data_sources: | |
| break | |
| for ds in data_sources: | |
| ds.mark_for_deletion(deleted_by=user_id) | |
| await self._ds_repo.delete(ds) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/management/application/services/knowledge_graph_service.py` around
lines 390 - 395, The current deletion uses a single fetch with limit=10000 (in
knowledge_graph_service.py) which can leave data sources undeleted; change the
logic around self._ds_repo.find_by_knowledge_graph and deletion so you page
through results until none remain: repeatedly call find_by_knowledge_graph
(using a reasonable page_size/limit) and for each returned batch call
ds.mark_for_deletion(...) and await self._ds_repo.delete(ds) for each item,
looping (incrementing offset or using the repo’s cursor) until the fetched batch
is empty; ensure this loop runs before deleting the KG to avoid integrity
errors.
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.
Avoid writing secrets before the database write is known to succeed.
At Line 167 and Line 339,
_secret_store.store()runs before the database work has been flushed/committed. Ifsave()or commit then fails, create leaves an orphaned secret and update can rotate credentials even though the request returns an error. It also keeps the transaction open across external I/O. Flush first, then write the secret, and add compensation if anything after the secret write fails.Also applies to: 337-347
🤖 Prompt for AI Agents