Skip to content

Conversation

abhinav-1305
Copy link

@abhinav-1305 abhinav-1305 commented Jul 2, 2025

PR Type

Enhancement

resolves: #1486


Description

  • Add delete project functionality with database query and API endpoint

  • Integrate projects management commands in CLI interface

  • Include comprehensive test coverage for project deletion scenarios

  • Implement safety checks preventing default project deletion


Changes diagram

flowchart LR
  A["Database Query"] --> B["API Endpoint"]
  B --> C["CLI Command"]
  D["Test Suite"] --> A
  E["Safety Checks"] --> A
Loading

Changes walkthrough 📝

Relevant files
Enhancement
7 files
__init__.py
Export delete_project function                                                     
+2/-0     
delete_project.py
Implement project deletion database query                               
+83/-0   
__init__.py
Export delete_project router function                                       
+1/-0     
delete_project.py
Add DELETE endpoint for projects                                                 
+30/-0   
__init__.py
Export projects_app module                                                             
+2/-0     
app.py
Register projects CLI subcommand                                                 
+5/-0     
projects.py
Implement project CLI commands                                                     
+107/-0 
Tests
1 files
test_project_delete.py
Add comprehensive project deletion tests                                 
+69/-0   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    SQL Injection

    The SQL query uses string concatenation with user input in the DO block. While parameters are used in the main DELETE statements, the error messages and logic checks could potentially be exploited if not properly sanitized.

    delete_project_query = """
    -- First check if the project exists and is not the default project
    DO $$
    BEGIN
        IF NOT EXISTS (
            SELECT 1 FROM projects 
            WHERE developer_id = $1 AND project_id = $2
        ) THEN
            RAISE EXCEPTION 'Project not found';
        END IF;
    
        IF EXISTS (
            SELECT 1 FROM projects 
            WHERE developer_id = $1 AND project_id = $2 AND canonical_name = 'default'
        ) THEN
            RAISE EXCEPTION 'Cannot delete default project';
        END IF;
    END $$;
    
    -- Delete all project associations to handle RESTRICT constraints
    DELETE FROM project_agents WHERE project_id = $2 AND developer_id = $1;
    DELETE FROM project_users WHERE project_id = $2 AND developer_id = $1;
    DELETE FROM project_files WHERE project_id = $2 AND developer_id = $1;
    
    -- Then delete the project itself
    DELETE FROM projects
    WHERE developer_id = $1
    AND project_id = $2
    RETURNING project_id;
    """
    Test Coverage

    Tests are missing verification for the default project deletion prevention. The test should specifically attempt to delete a project with canonical_name='default' and verify the appropriate exception is raised.

    @pytest.mark.asyncio
    async def test_delete_project_not_found(dsn=pg_dsn, developer=test_developer):
        """Test that deleting a non-existent project raises an appropriate error."""
        non_existent_project_id = uuid4()
    
        with pytest.raises(Exception) as exc_info:
            await delete_project(
                developer_id=developer.developer_id,
                project_id=non_existent_project_id
            )
    
        # The exact exception type and message may vary based on the database layer
        assert exc_info.value is not None
    
    
    @pytest.mark.asyncio
    async def test_delete_project_wrong_developer(dsn=pg_dsn, developer=test_developer, project=test_project):
        """Test that deleting a project with wrong developer ID raises an error."""
        wrong_developer_id = uuid4()
    
        with pytest.raises(Exception) as exc_info:
            await delete_project(
                developer_id=wrong_developer_id,
                project_id=project.id
            )
    
        # The exact exception type and message may vary based on the database layer
        assert exc_info.value is not None 
    Error Handling

    The CLI delete command catches all exceptions generically without distinguishing between different error types like 'project not found', 'default project deletion', or network errors, making debugging difficult for users.

    try:
        client.projects.delete(project_id)
        progress.update(delete_project_task, completed=True)
    except Exception as e:
        progress.update(delete_project_task, completed=True)
        error_console.print(Text(f"Failed to delete project: {e}", style="bold red"))
        raise typer.Exit(1)

    Copy link
    Contributor

    qodo-merge-for-open-source bot commented Jul 2, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Wrap operations in transaction
    Suggestion Impact:The suggestion was directly implemented - the commit added BEGIN and COMMIT statements to wrap the entire query in a transaction, along with a comment explaining the purpose

    code diff:

    +# Wrapped in a transaction to ensure atomicity
     delete_project_query = """
    +BEGIN;
    +
     -- First check if the project exists and is not the default project
     DO $$
     BEGIN
    @@ -43,6 +46,8 @@
     WHERE developer_id = $1
     AND project_id = $2
     RETURNING project_id;
    +
    +COMMIT;

    The query performs multiple checks and deletions but doesn't wrap them in a
    transaction. If any of the DELETE operations fail after the initial checks pass,
    the database could be left in an inconsistent state. Wrap the entire operation
    in a transaction to ensure atomicity.

    src/agents-api/agents_api/queries/projects/delete_project.py [17-46]

     delete_project_query = """
    +BEGIN;
    +
     -- First check if the project exists and is not the default project
     DO $$
     BEGIN
         IF NOT EXISTS (
             SELECT 1 FROM projects 
             WHERE developer_id = $1 AND project_id = $2
         ) THEN
             RAISE EXCEPTION 'Project not found';
         END IF;
         
         IF EXISTS (
             SELECT 1 FROM projects 
             WHERE developer_id = $1 AND project_id = $2 AND canonical_name = 'default'
         ) THEN
             RAISE EXCEPTION 'Cannot delete default project';
         END IF;
     END $$;
     
     -- Delete all project associations to handle RESTRICT constraints
     DELETE FROM project_agents WHERE project_id = $2 AND developer_id = $1;
     DELETE FROM project_users WHERE project_id = $2 AND developer_id = $1;
     DELETE FROM project_files WHERE project_id = $2 AND developer_id = $1;
     
     -- Then delete the project itself
     DELETE FROM projects
     WHERE developer_id = $1
     AND project_id = $2
     RETURNING project_id;
    +
    +COMMIT;
     """

    [Suggestion processed]

    Suggestion importance[1-10]: 9

    __

    Why: The suggestion correctly identifies a critical data integrity issue where multiple DELETE operations are not wrapped in a transaction, which could lead to an inconsistent database state upon failure.

    High
    General
    Validate UUID format
    Suggestion Impact:The suggestion was implemented by changing the type annotation from str to UUID, which provides automatic UUID validation through the type system instead of manual validation

    code diff:

    -    project_id: Annotated[str, typer.Option("--id", help="ID of the project to delete")],
    +    project_id: Annotated[UUID, typer.Option("--id", help="ID of the project to delete")],

    The project_id parameter is typed as str but should be validated as a UUID since
    the API expects a UUID. Invalid UUID strings will cause runtime errors when
    passed to the client. Add UUID validation or change the type annotation.

    src/cli/src/julep_cli/projects.py [17-23]

     def delete(
         project_id: Annotated[str, typer.Option("--id", help="ID of the project to delete")],
         yes: Annotated[
             bool,
             typer.Option("--yes", "-y", help="Skip confirmation prompt"),
         ] = False,
     ):
    +    # Validate UUID format
    +    try:
    +        UUID(project_id)
    +    except ValueError:
    +        error_console.print(Text(f"Invalid project ID format: {project_id}", style="bold red"))
    +        raise typer.Exit(1)

    [Suggestion processed]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that the project_id string from the CLI should be validated as a UUID before being passed to the API client, improving robustness and user experience.

    Medium
    • Update

    Copy link
    Contributor

    @ellipsis-dev ellipsis-dev bot left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Caution

    Changes requested ❌

    Reviewed everything up to 72588f8 in 1 minute and 31 seconds. Click for details.
    • Reviewed 385 lines of code in 8 files
    • Skipped 0 files when reviewing.
    • Skipped posting 2 draft comments. View those below.
    • Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
    1. src/agents-api/agents_api/queries/projects/delete_project.py:83
    • Draft comment:
      Missing newline at end of file.
    • Reason this comment was not posted:
      Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide a specific code suggestion or ask for a specific test to be written. It does not align with the rules for good comments, as it does not address a potential issue or improvement in the code logic.
    2. src/agents-api/tests/test_project_delete.py:47
    • Draft comment:
      Consider checking for a more specific exception (or its message) instead of using the generic Exception for better test precision.
    • Reason this comment was not posted:
      Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The code explicitly comments that exception types may vary based on the database layer, suggesting this generic catch is intentional. Making the exception more specific could make the tests brittle if the underlying database implementation changes. The current approach seems to be a deliberate design choice rather than an oversight. The comment does raise a valid testing best practice of being specific about exceptions. The current code might miss catching wrong exception types. The intentional flexibility in exception handling appears to be a conscious architectural decision to handle database layer variations, making specific exception checking potentially counterproductive. Delete the comment as the generic exception handling appears to be an intentional design choice documented in the code comments.

    Workflow ID: wflow_Gh8Gbyo0gbxVp0Gk

    You can customize Ellipsis by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

    @abhinav-1305
    Copy link
    Author

    please review - @Ahmad-mtos

    @creatorrr
    Copy link
    Contributor

    Thanks for working on this @abhinav-1305 . We'll review and get back shortly. cc/ @anasalatasi @anasalatasiuni @Ahmad-mtos

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [Bug]: Not able to delete a deployed template project
    2 participants