Skip to content
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

refactor(platform/rest): Update REST API to use standard FastAPI structure #8519

Merged

Conversation

Swiftyos
Copy link
Contributor

@Swiftyos Swiftyos commented Nov 1, 2024

Background

The rest FastAPI server is currently packaged up and ran from within a separate process. This has resulted in needing to develop the FastAPI server in a unique way, preventing us from leveraging some of its features and slightly increasing the difficulty of adding new routes.

Work has been done to decouple the Rest server from the executors. This is the follow on work to refactor the API server to a canonical FastAPI setup.

Changes 🏗️

  • Moved to a standard FastAPI server, ran via uvicorn directly, in the docker container.

  • Ensured openapi schemas matched exactly:

    Added python script to ensure openapi specs do not change during refactor

     % python comp.py                                     
    The two OpenAPI specifications are identical.
    
  • Moved endpoints from /api/x to /api/v1/x

Testing 🔍

Note

Only for the new autogpt platform, currently in autogpt_platform/

  • Create from scratch and execute an agent with at least 3 blocks
  • Import an agent from file upload, and confirm it executes correctly
  • Upload agent to marketplace
  • Import an agent from marketplace and confirm it executes correctly
  • Edit an agent from monitor, and confirm it executes correctly

Configuration Changes 📝

Note

Only for the new autogpt platform, currently in autogpt_platform/

If you're making configuration or infrastructure changes, please remember to check you've updated the related infrastructure code in the autogpt_platform/infra folder.

Examples of such changes might include:

  • Changing ports
  • Adding new services that need to communicate with each other
  • Secrets or environment variable changes
  • New or infrastructure changes such as databases

@Swiftyos Swiftyos requested a review from a team as a code owner November 1, 2024 12:09
@Swiftyos Swiftyos requested review from Pwuts and kcze and removed request for a team November 1, 2024 12:09
@Swiftyos Swiftyos changed the base branch from master to dev November 1, 2024 12:10
Copy link
Contributor

github-actions bot commented Nov 1, 2024

This PR targets the master branch but does not come from dev or a hotfix/* branch.

Automatically setting the base branch to dev.

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

CORS configuration:
The CORS middleware in autogpt_platform/backend/backend/server/routers/v1.py is configured to allow all origins, methods, and headers. This could potentially lead to security vulnerabilities if not properly restricted in production. It's recommended to specify allowed origins, methods, and headers explicitly rather than using wildcard "*" for all.

⚡ Recommended focus areas for review

Error Handling
The global exception handler might be too broad. It catches all exceptions and returns a 500 status code, which could mask specific errors that should be handled differently.

Security Concern
The CORS middleware is configured to allow all origins, methods, and headers. This could potentially lead to security vulnerabilities if not properly restricted in production.

Error Handling
The error handling in the execute_graph function could be improved. It's catching all exceptions and returning a 400 status code, which might not be appropriate for all types of errors.

Copy link

netlify bot commented Nov 1, 2024

Deploy Preview for auto-gpt-docs canceled.

Name Link
🔨 Latest commit 69fcf2f
🔍 Latest deploy log https://app.netlify.com/sites/auto-gpt-docs/deploys/6724c5131b1c07000841043b

@github-actions github-actions bot added the platform/frontend AutoGPT Platform - Front end label Nov 1, 2024
@Swiftyos Swiftyos requested a review from majdyz November 1, 2024 16:32
@Swiftyos Swiftyos enabled auto-merge (squash) November 4, 2024 08:49
Copy link
Contributor

@majdyz majdyz left a comment

Choose a reason for hiding this comment

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

codewise LGTM,
I have no opinion on v1 routing, is this something that we want to do?
Should we not retain the default one to point to the latest version to maintain backward compatibility?

@Swiftyos Swiftyos merged commit 594aa99 into dev Nov 4, 2024
10 checks passed
@Swiftyos Swiftyos deleted the swiftyos/secrt-861-update-rest-api-to-use-a-standard-structure_2 branch November 4, 2024 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform/backend AutoGPT Platform - Back end platform/frontend AutoGPT Platform - Front end Review effort [1-5]: 4 size/xl
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants