mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
More refactors of public -> shared
This commit is contained in:
parent
57c71232a7
commit
734f5cd983
@ -1,95 +0,0 @@
|
||||
# Conversation Sharing Refactoring Summary
|
||||
|
||||
## Overview
|
||||
This refactoring changes the conversation sharing functionality from "public" terminology to "shared" terminology to make it more generic and future-proof for different sharing scenarios (public sharing, user-specific sharing, etc.).
|
||||
|
||||
## Files Renamed
|
||||
|
||||
### Core Service Files
|
||||
- `openhands/app_server/sharing/public_conversation_models.py` → `shared_conversation_models.py`
|
||||
- `openhands/app_server/sharing/public_conversation_info_service.py` → `shared_conversation_info_service.py`
|
||||
- `openhands/app_server/sharing/sql_public_conversation_info_service.py` → `sql_shared_conversation_info_service.py`
|
||||
- `openhands/app_server/sharing/public_event_service.py` → `shared_event_service.py`
|
||||
- `openhands/app_server/sharing/public_event_service_impl.py` → `shared_event_service_impl.py`
|
||||
|
||||
### Router Files
|
||||
- `openhands/app_server/sharing/public_conversation_router.py` → `shared_conversation_router.py`
|
||||
- `openhands/app_server/sharing/public_event_router.py` → `shared_event_router.py`
|
||||
|
||||
### Test Files
|
||||
- `tests/unit/test_sharing/test_public_conversation_models.py` → `test_shared_conversation_models.py`
|
||||
- `tests/unit/test_sharing_public_conversation_info_service.py` → `test_sharing_shared_conversation_info_service.py`
|
||||
- `tests/unit/test_sharing_public_event_service.py` → `test_sharing_shared_event_service.py`
|
||||
|
||||
## Classes Renamed
|
||||
|
||||
### Model Classes
|
||||
- `PublicConversation` → `SharedConversation`
|
||||
- `PublicConversationSortOrder` → `SharedConversationSortOrder`
|
||||
- `PublicConversationPage` → `SharedConversationPage`
|
||||
|
||||
### Service Classes
|
||||
- `PublicConversationInfoService` → `SharedConversationInfoService`
|
||||
- `PublicConversationInfoServiceInjector` → `SharedConversationInfoServiceInjector`
|
||||
- `SQLPublicConversationInfoService` → `SQLSharedConversationInfoService`
|
||||
- `SQLPublicConversationInfoServiceInjector` → `SQLSharedConversationInfoServiceInjector`
|
||||
- `PublicEventService` → `SharedEventService`
|
||||
- `PublicEventServiceInjector` → `SharedEventServiceInjector`
|
||||
- `PublicEventServiceImpl` → `SharedEventServiceImpl`
|
||||
- `PublicEventServiceImplInjector` → `SharedEventServiceImplInjector`
|
||||
|
||||
## API Endpoints Updated
|
||||
|
||||
### Conversation Endpoints
|
||||
- `/api/v1/public-conversations` → `/api/v1/shared-conversations`
|
||||
- `/api/v1/public-conversations/{conversation_id}` → `/api/v1/shared-conversations/{conversation_id}`
|
||||
|
||||
### Event Endpoints
|
||||
- `/api/v1/public-events` → `/api/v1/shared-events`
|
||||
- `/api/v1/public-events/search` → `/api/v1/shared-events/search`
|
||||
|
||||
## Configuration Changes
|
||||
|
||||
### Dependency Functions
|
||||
- `depends_public_conversation_info_service()` → `depends_shared_conversation_info_service()`
|
||||
- `depends_public_event_service()` → `depends_shared_event_service()`
|
||||
- `get_public_conversation_info_service()` → `get_shared_conversation_info_service()`
|
||||
- `get_public_event_service()` → `get_shared_event_service()`
|
||||
|
||||
### Configuration Attributes
|
||||
- `config.public_conversation_info` → `config.shared_conversation_info`
|
||||
- `config.public_event` → `config.shared_event`
|
||||
|
||||
## Method Names Updated
|
||||
|
||||
### Service Methods
|
||||
- `get_public_conversations()` → `get_shared_conversations()`
|
||||
- `get_public_conversation()` → `get_shared_conversation()`
|
||||
- `search_public_events()` → `search_shared_events()`
|
||||
- `get_public_events()` → `get_shared_events()`
|
||||
|
||||
## Database Schema
|
||||
**Note**: The database column name `public` was intentionally kept unchanged to avoid breaking existing data. This provides backward compatibility while the codebase transitions to the new terminology.
|
||||
|
||||
## Future Considerations
|
||||
|
||||
### Extending Sharing Functionality
|
||||
With this refactoring, the codebase is now prepared for extending sharing functionality:
|
||||
|
||||
1. **User-Specific Sharing**: Add fields like `shared_with_users` or `sharing_permissions` to the `SharedConversation` model
|
||||
2. **Sharing Types**: Add an enum field `sharing_type` with values like `PUBLIC`, `PRIVATE`, `SPECIFIC_USERS`
|
||||
3. **Access Control**: Implement permission checks in the service layer based on sharing type and user context
|
||||
|
||||
### Recommended Next Steps
|
||||
1. **Database Migration**: Plan a future migration to rename the `public` column to `shared` or `is_shared`
|
||||
2. **Frontend Updates**: Update frontend components to use the new API endpoints
|
||||
3. **Documentation**: Update API documentation to reflect the new endpoint names
|
||||
4. **Monitoring**: Update any monitoring or logging that references the old names
|
||||
|
||||
## Backward Compatibility
|
||||
- Database schema remains unchanged (column still named `public`)
|
||||
- All functionality preserved with new naming
|
||||
- API endpoints changed (breaking change for clients)
|
||||
|
||||
## Testing
|
||||
All test files have been updated and renamed to match the new terminology. The test suite should continue to pass with the new names.
|
||||
@ -1,20 +1,20 @@
|
||||
# Sharing Package
|
||||
|
||||
This package contains functionality for sharing conversations publicly.
|
||||
This package contains functionality for sharing conversations.
|
||||
|
||||
## Components
|
||||
|
||||
- **public_conversation_models.py**: Data models for public conversations
|
||||
- **public_conversation_info_service.py**: Service interface for accessing public conversation info
|
||||
- **sql_public_conversation_info_service.py**: SQL implementation of the public conversation info service
|
||||
- **public_event_service.py**: Service interface for accessing public events
|
||||
- **public_event_service_impl.py**: Implementation of the public event service
|
||||
- **public_conversation_router.py**: REST API endpoints for public conversations
|
||||
- **public_event_router.py**: REST API endpoints for public events
|
||||
- **shared.py**: Data models for shared conversations
|
||||
- **shared_conversation_info_service.py**: Service interface for accessing shared conversation info
|
||||
- **sql_shared_conversation_info_service.py**: SQL implementation of the shared conversation info service
|
||||
- **shared_event_service.py**: Service interface for accessing shared events
|
||||
- **shared_event_service_impl.py**: Implementation of the shared event service
|
||||
- **shared_conversation_router.py**: REST API endpoints for shared conversations
|
||||
- **shared_event_router.py**: REST API endpoints for shared events
|
||||
|
||||
## Features
|
||||
|
||||
- Read-only access to public conversations
|
||||
- Event access for public conversations
|
||||
- Read-only access to shared conversations
|
||||
- Event access for shared conversations
|
||||
- Search and filtering capabilities
|
||||
- Pagination support
|
||||
|
||||
@ -28,7 +28,7 @@ router.shared_conversation_service_dependency = shared_conversation_service_depe
|
||||
|
||||
|
||||
@router.get('/search')
|
||||
async def search_public_conversations(
|
||||
async def search_shared_conversations(
|
||||
title__contains: Annotated[
|
||||
str | None,
|
||||
Query(title='Filter by title containing this string'),
|
||||
@ -73,10 +73,10 @@ async def search_public_conversations(
|
||||
] = False,
|
||||
shared_conversation_service: SharedConversationInfoService = shared_conversation_service_dependency,
|
||||
) -> SharedConversationPage:
|
||||
"""Search / List public conversations."""
|
||||
"""Search / List shared conversations."""
|
||||
assert limit > 0
|
||||
assert limit <= 100
|
||||
return await shared_conversation_service.search_public_conversation_info(
|
||||
return await shared_conversation_service.search_shared_conversation_info(
|
||||
title__contains=title__contains,
|
||||
created_at__gte=created_at__gte,
|
||||
created_at__lt=created_at__lt,
|
||||
@ -90,7 +90,7 @@ async def search_public_conversations(
|
||||
|
||||
|
||||
@router.get('/count')
|
||||
async def count_public_conversations(
|
||||
async def count_shared_conversations(
|
||||
title__contains: Annotated[
|
||||
str | None,
|
||||
Query(title='Filter by title containing this string'),
|
||||
@ -113,7 +113,7 @@ async def count_public_conversations(
|
||||
] = None,
|
||||
shared_conversation_service: SharedConversationInfoService = shared_conversation_service_dependency,
|
||||
) -> int:
|
||||
"""Count public conversations matching the given filters."""
|
||||
"""Count shared conversations matching the given filters."""
|
||||
return await shared_conversation_service.count_shared_conversation_info(
|
||||
title__contains=title__contains,
|
||||
created_at__gte=created_at__gte,
|
||||
@ -124,14 +124,14 @@ async def count_public_conversations(
|
||||
|
||||
|
||||
@router.get('')
|
||||
async def batch_get_public_conversations(
|
||||
async def batch_get_shared_conversations(
|
||||
ids: Annotated[list[str], Query()],
|
||||
shared_conversation_service: SharedConversationInfoService = shared_conversation_service_dependency,
|
||||
) -> list[SharedConversation | None]:
|
||||
"""Get a batch of public conversations given their ids. Return None for any missing or non-public."""
|
||||
"""Get a batch of shared conversations given their ids. Return None for any missing or non-shared."""
|
||||
assert len(ids) <= 100
|
||||
uuids = [UUID(id_) for id_ in ids]
|
||||
public_conversations = (
|
||||
shared_conversation_info = (
|
||||
await shared_conversation_service.batch_get_shared_conversation_info(uuids)
|
||||
)
|
||||
return public_conversations
|
||||
return shared_conversation_info
|
||||
|
||||
@ -55,7 +55,7 @@ async def search_shared_events(
|
||||
] = 100,
|
||||
shared_event_service: SharedEventService = shared_event_service_dependency,
|
||||
) -> EventPage:
|
||||
"""Search / List events for a public conversation."""
|
||||
"""Search / List events for a shared conversation."""
|
||||
assert limit > 0
|
||||
assert limit <= 100
|
||||
return await shared_event_service.search_shared_events(
|
||||
@ -70,7 +70,7 @@ async def search_shared_events(
|
||||
|
||||
|
||||
@router.get('/count')
|
||||
async def count_public_events(
|
||||
async def count_shared_events(
|
||||
conversation_id: Annotated[
|
||||
UUID,
|
||||
Query(title='Conversation ID to count events for'),
|
||||
@ -93,8 +93,8 @@ async def count_public_events(
|
||||
] = EventSortOrder.TIMESTAMP,
|
||||
shared_event_service: SharedEventService = shared_event_service_dependency,
|
||||
) -> int:
|
||||
"""Count events for a public conversation matching the given filters."""
|
||||
return await shared_event_service.count_public_events(
|
||||
"""Count events for a shared conversation matching the given filters."""
|
||||
return await shared_event_service.count_shared_events(
|
||||
conversation_id=conversation_id,
|
||||
kind__eq=kind__eq,
|
||||
timestamp__gte=timestamp__gte,
|
||||
@ -112,17 +112,17 @@ async def batch_get_shared_events(
|
||||
id: Annotated[list[str], Query()],
|
||||
shared_event_service: SharedEventService = shared_event_service_dependency,
|
||||
) -> list[Event | None]:
|
||||
"""Get a batch of events for a public conversation given their ids, returning null for any missing event."""
|
||||
"""Get a batch of events for a shared conversation given their ids, returning null for any missing event."""
|
||||
assert len(id) <= 100
|
||||
events = await shared_event_service.batch_get_shared_events(conversation_id, id)
|
||||
return events
|
||||
|
||||
|
||||
@router.get('/{conversation_id}/{event_id}')
|
||||
async def get_public_event(
|
||||
async def get_shared_event(
|
||||
conversation_id: UUID,
|
||||
event_id: str,
|
||||
shared_event_service: SharedEventService = shared_event_service_dependency,
|
||||
) -> Event | None:
|
||||
"""Get a single event from a public conversation by conversation_id and event_id."""
|
||||
return await shared_event_service.get_public_event(conversation_id, event_id)
|
||||
"""Get a single event from a shared conversation by conversation_id and event_id."""
|
||||
return await shared_event_service.get_shared_event(conversation_id, event_id)
|
||||
|
||||
@ -34,9 +34,9 @@ logger = logging.getLogger(__name__)
|
||||
|
||||
@dataclass
|
||||
class SharedEventServiceImpl(SharedEventService):
|
||||
"""Implementation of SharedEventService that validates public access."""
|
||||
"""Implementation of SharedEventService that validates shared access."""
|
||||
|
||||
public_conversation_service: SharedConversationInfoService
|
||||
shared_conversation_info_service: SharedConversationInfoService
|
||||
event_service: EventService
|
||||
|
||||
async def get_shared_event(
|
||||
@ -45,7 +45,7 @@ class SharedEventServiceImpl(SharedEventService):
|
||||
"""Given a conversation_id and event_id, retrieve an event if the conversation is shared."""
|
||||
# First check if the conversation is shared
|
||||
public_conversation = (
|
||||
await self.public_conversation_service.get_public_conversation_info(
|
||||
await self.shared_conversation_info_service.get_shared_conversation_info(
|
||||
conversation_id
|
||||
)
|
||||
)
|
||||
@ -67,12 +67,12 @@ class SharedEventServiceImpl(SharedEventService):
|
||||
) -> EventPage:
|
||||
"""Search events for a specific shared conversation."""
|
||||
# First check if the conversation is shared
|
||||
public_conversation = (
|
||||
await self.public_conversation_service.get_public_conversation_info(
|
||||
shared_conversation_info = (
|
||||
await self.shared_conversation_info_service.get_shared_conversation_info(
|
||||
conversation_id
|
||||
)
|
||||
)
|
||||
if public_conversation is None:
|
||||
if shared_conversation_info is None:
|
||||
# Return empty page if conversation is not public
|
||||
return EventPage(items=[], next_page_id=None)
|
||||
|
||||
@ -97,12 +97,12 @@ class SharedEventServiceImpl(SharedEventService):
|
||||
) -> int:
|
||||
"""Count events for a specific shared conversation."""
|
||||
# First check if the conversation is shared
|
||||
public_conversation = (
|
||||
await self.public_conversation_service.get_public_conversation_info(
|
||||
shared_conversation_info = (
|
||||
await self.shared_conversation_info_service.get_shared_conversation_info(
|
||||
conversation_id
|
||||
)
|
||||
)
|
||||
if public_conversation is None:
|
||||
if shared_conversation_info is None:
|
||||
return 0
|
||||
|
||||
# If conversation is shared, count events for this conversation
|
||||
@ -128,11 +128,11 @@ class SharedEventServiceImplInjector(SharedEventServiceInjector):
|
||||
async with (
|
||||
get_shared_conversation_info_service(
|
||||
state, request
|
||||
) as public_conversation_service,
|
||||
) as shared_conversation_info_service,
|
||||
get_event_service(state, request) as event_service,
|
||||
):
|
||||
service = SharedEventServiceImpl(
|
||||
public_conversation_service=public_conversation_service,
|
||||
shared_conversation_info_service=shared_conversation_info_service,
|
||||
event_service=event_service,
|
||||
)
|
||||
yield service
|
||||
|
||||
@ -111,7 +111,7 @@ class SQLSharedConversationInfoService(SharedConversationInfoService):
|
||||
if has_more:
|
||||
rows = rows[:limit]
|
||||
|
||||
items = [self._to_public_conversation(row) for row in rows]
|
||||
items = [self._to_shared_conversation(row) for row in rows]
|
||||
|
||||
# Calculate next page ID
|
||||
next_page_id = None
|
||||
@ -128,11 +128,11 @@ class SQLSharedConversationInfoService(SharedConversationInfoService):
|
||||
updated_at__gte: datetime | None = None,
|
||||
updated_at__lt: datetime | None = None,
|
||||
) -> int:
|
||||
"""Count public conversations matching the given filters."""
|
||||
"""Count shared conversations matching the given filters."""
|
||||
from sqlalchemy import func
|
||||
|
||||
query = select(func.count(StoredConversationMetadata.conversation_id))
|
||||
# Only include public conversations
|
||||
# Only include shared conversations
|
||||
query = query.where(StoredConversationMetadata.public == True) # noqa: E712
|
||||
query = query.where(StoredConversationMetadata.conversation_version == 'V1')
|
||||
|
||||
@ -151,7 +151,7 @@ class SQLSharedConversationInfoService(SharedConversationInfoService):
|
||||
async def get_shared_conversation_info(
|
||||
self, conversation_id: UUID
|
||||
) -> SharedConversation | None:
|
||||
"""Get a single public conversation info, returning None if missing or not public."""
|
||||
"""Get a single public conversation info, returning None if missing or not shared."""
|
||||
query = self._public_select().where(
|
||||
StoredConversationMetadata.conversation_id == str(conversation_id)
|
||||
)
|
||||
@ -162,7 +162,7 @@ class SQLSharedConversationInfoService(SharedConversationInfoService):
|
||||
if stored is None:
|
||||
return None
|
||||
|
||||
return self._to_public_conversation(stored)
|
||||
return self._to_shared_conversation(stored)
|
||||
|
||||
def _public_select(self):
|
||||
"""Create a select query that only returns public conversations."""
|
||||
@ -208,7 +208,7 @@ class SQLSharedConversationInfoService(SharedConversationInfoService):
|
||||
|
||||
return query
|
||||
|
||||
def _to_public_conversation(
|
||||
def _to_shared_conversation(
|
||||
self,
|
||||
stored: StoredConversationMetadata,
|
||||
sub_conversation_ids: list[UUID] | None = None,
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user