Feature/document module/dev #114
Conversation
* Basic document model * Document route skeleton Implements document list based on current user. * Imports for document route exposure * Add relationship between users and documents
* Cleaner syntax * Interface for cloud storage functionality * Create module dedicated to cloud functionality * Take the user in the constructor * Rename the document list route Route names will conform to common Unix operations. * Implementation of file upload * Default placeholders for AWS variables * Take AWS credentials from .env * Perform bucket creation at startup * Use a single bucket for all clients #59 (comment) * Changes to support new bucket semantics * Since there is a single bucket per client: storage class no longer responsible for name generation * Since the bucket is created at platform startup: storage class no longer needs to worry about it, and client creation needs to be globally accessible. * Missing imports * Unused imports * Lift timestamp generation to common location * Timestamp generation is global resource * Updating a document is not yet supported * Flesh out document remove and stat * Must specify region when creating a bucket See: https://stackoverflow.com/a/49665620 * Add boto3 requirement * Repeating AWS environment variables in the settings * Client expected to pass the basename of the destination Storage class builds bucket path based on user information and the basename. This allows the basename to follow the clients convention. * Corrected upload file specification * Build basename that matches the UUID expectation of the model * SQLAlchemy cannot process Path types natively * Ensure document ID is passed to route body * Corrected database interaction when deleting * More graceful handling of non singular results * Move document database interactions to CRUD * Ignore Emacs backup files * Allow document list to be iterable * Corrected parameter naming * Initial document CRUD tests * Test for read_* methods * Infrastructure for making it work (utils) * Document update returns inserted document * Lift document creation function * Test document update * Whitespace * Linted * Corrected update_at test * Appropriate class variable naming * Move document creation into a class * Linted * Test document CRUD delete * All test methods cleanup after themselves * Gracefully handle negative skip's and limit's * Fixture usage is more explicit and straightforward * Better usage of fixtures * Import reordering * Lift document crud testing utils to global testing utilities * Take read error into account * Test document list route * Simplify test parameters * Lift common document endpoint testing resources to utils * Return number of rows deleted * Test document endpoint deletion * Consisten Session variable naming * Special list document type * Ability to add component to URL path * Corrections to injected types * Simplification of Route semantics * Route type now handles all URL operations required for the crawler * Crawler assumes it will be called with a single type (Route) * Linted * Delete uses update * Lift document comparison to test utilities * Tests assert * Must refresh the session before interacting with the database * Tests for document stat route * Linted * Better temporary bucket naming * Move from deprecated way of Pydantic to dict * Lift bucket creation to global module * Unused import * Return all information about an uploaded document * Updates to Python packages * Bump boto3 version * Add moto dependency * Test upload endpoint * Use object to upload documents * General document test cleanup * Fewer fixtures that are more general * Remove class fixtures; assume someone else manages database state * Consolidate manual database interaction into a single class * Fix test class name clashes * Remove unused imports * Remove extraneous code * Document crud takes respects user throughout Each method takes into account whether the client is allowed to access the documents they are trying to touch. To do this, owner is dropped from the method signatures; instead being specified at construction. * Routes respect new document CRUD interface * Integer to UUID now a standalone generator * Better variable naming * Tests take into account new document CRUD interface * Unused imports * More descriptive AWS error type * Catch generic exceptions to ensure service does not go down * Ensure boto3 respects environment The preference for client creation is to take parameters values from the environment; platform "settings" are a backup. This ensures that when mocking S3, things work as expected. * Log cloud storage errors during startup * Corrected variable naming
jerome-white
requested review from
AkhileshNegi,
avirajsingh7,
nishika26 and
sourabhlodha
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kody-ai[bot]](https://avatars.githubusercontent.com/in/413034?s=60&v=4){kind=small}
| from app.api.routes import ( | ||
| api_keys, | ||
| documents, | ||
| items, | ||
| login, | ||
| organization, | ||
| project, | ||
| project_user, | ||
| private, | ||
| threads, | ||
| users, | ||
| utils, | ||
| ) |
There was a problem hiding this comment.
from app.api.routes import (
api_keys,
documents,
items,
login,
organization,
private,
project,
project_user,
threads,
users,
utils,
)Multiple instances of missing type hints or unclear type definitions reduce code maintainability and IDE support.
This issue appears in multiple locations:
- backend/app/api/main.py: Lines 3-15
- backend/app/core/util.py: Lines 3-3
- backend/app/models/document.py: Lines 29-30
Please add type hints and clear type definitions to improve code clarity and maintainability.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| api_router.include_router(login.router) | ||
| api_router.include_router(users.router) | ||
| api_router.include_router(utils.router) | ||
| api_router.include_router(items.router) | ||
| api_router.include_router(documents.router) | ||
| api_router.include_router(threads.router) | ||
| api_router.include_router(organization.router) | ||
| api_router.include_router(project.router) | ||
| api_router.include_router(project_user.router) | ||
| api_router.include_router(api_keys.router) |
There was a problem hiding this comment.
# Auth routes
api_router.include_router(login.router)
api_router.include_router(users.router)
# Document management routes
api_router.include_router(documents.router)
api_router.include_router(threads.router)
# Organization routes
api_router.include_router(organization.router)
api_router.include_router(project.router)
api_router.include_router(project_user.router)
# Utility routes
api_router.include_router(utils.router)
api_router.include_router(items.router)
api_router.include_router(api_keys.router)Group related routers together in the inclusion order for better code organization and maintenance. For example, group authentication-related routers (login, users), document-related routers (documents, threads), and organization-related routers (organization, project, project_user) together.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| except Exception as err: | ||
| raise_from_unknown(err) | ||
|
|
||
| @router.get("/rm/{doc_id}") |
There was a problem hiding this comment.
@router.delete("/rm/{doc_id}")HTTP DELETE method should be used for the delete operation instead of GET, as per REST conventions. GET requests should be idempotent and not modify server state.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| @computed_field # type: ignore[prop-decorator] | ||
| @property | ||
| def AWS_S3_BUCKET(self) -> str: | ||
| return f'ai-platform-documents-{self.ENVIRONMENT}' |
There was a problem hiding this comment.
@computed_field # type: ignore[prop-decorator]
@property
def AWS_S3_BUCKET(self) -> str:
bucket_name = f'ai-platform-documents-{self.ENVIRONMENT}'.lower()
if not bucket_name.islower() or not all(c.isalnum() or c in '-.' for c in bucket_name):
raise ValueError('S3 bucket name must contain only lowercase letters, numbers, hyphens, and periods')
return bucket_nameThe S3 bucket name could potentially contain invalid characters from the environment variable. Add validation to ensure the bucket name follows AWS S3 naming conventions.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| owner_id: UUID = Field( | ||
| foreign_key='user.id', | ||
| nullable=False, | ||
| ondelete='CASCADE', | ||
| ) |
There was a problem hiding this comment.
owner_id: UUID = Field(
foreign_key='user.id',
nullable=False,
ondelete='CASCADE',
index=True,
)Add an index on owner_id to improve query performance when fetching documents by owner, which is likely a common operation.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| ): | ||
| crud = DocumentCrud(session, current_user.id) | ||
| try: | ||
| return crud.read_many(skip, limit) |
There was a problem hiding this comment.
We are implementing a standardized API response format across all API responses to ensure consistency throughout the application. You can refer to the implementation details in Pull Request #67.
Additionally, we can now add metadata in the response if required
Any HTTPException raised will be automatically converted to the standardized format, maintaining uniformity across all responses.
There was a problem hiding this comment.
The goal of this PR is to bring my code in line with main. Given that, is it okay if I do that as a new PR once this is merged? I'm not even sure, for example, that this branch has the standard response format definitions. I've been away for too long
| ) | ||
|
|
||
| try: | ||
| return crud.update(document) |
There was a problem hiding this comment.
Need to add standard api response for all the endpoint
| from .user import User | ||
|
|
||
| class Document(SQLModel, table=True): | ||
| id: UUID = Field( |
There was a problem hiding this comment.
We will be using id as int for all entities.
The Project and Organization models are already set up this way, but uuid was previously used for the user_id as part of the default template. To address this, I have created an issue to convert user_id to int for consistency.
There was a problem hiding this comment.
I prefer int's, so I welcome this. However, as with the other change, is it okay if we do this after the merge? Making this change will have bigger implications for how some of the other document-related stuff work
There was a problem hiding this comment.
No worries, @jerome-white. I am approving the PR.
You can create an issue to ensure this doesn't get ignored, and make sure to include the lock file, as it's essential.
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Kody Review CompleteGreat news! 🎉 Keep up the excellent work! 🚀 Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
Summary
Target issue is #113
The document module had been developed against this branch. This merges those additions into main; and lays the groundwork for continuing document module development more closely tied to the main branch.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
All code was reviewed in previous issues:
This pull request introduces a new document management module to the
ai-platformproject. Key changes include:.gitignore Enhancements: Added entries to exclude macOS and Emacs temporary files, improving project maintenance by keeping the repository clean from unnecessary files.
API Enhancements:
main.pyfor better structure.routes/documents.pyfor document management, featuring CRUD operations, cloud storage integration, and error handling.Cloud Storage Integration:
storage.py, including client configuration, bucket management, and file upload capabilities.config.pyfor S3 bucket integration, including access credentials and environment-based bucket name computation.initial_storage.pyfor initializing cloud storage with error handling and logging.Model and CRUD Updates:
DocumentandDocumentListmodels indocument.pyusing SQLModel, defining the database schema with UUID-based identification and user ownership.user.pyto include a documents relationship and a count field in theUsersPublicclass.models/__init__.py.Utility and Script Modifications:
util.pyto get the current UTC time without timezone information.prestart.shscript to handle multiple initialization services using an array-based approach.These changes enhance the document management capabilities of the platform, integrate cloud storage, and improve overall project organization and initialization processes.