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

FEAT: Enhance API Deployment Validation to Return 4xx Instead of 500 for Errors #890

Merged
merged 22 commits into from
Dec 17, 2024

Conversation

pk-zipstack
Copy link
Contributor

@pk-zipstack pk-zipstack commented Dec 11, 2024

What

  • Refactored WorkflowHelper to handle execution errors: Added explicit exception handling for non-existent execution_id
  • Updated serializers.py: Added validation for execution_id to ensure it adheres to a valid UUID format.
  • Enhanced error management in api_deployment_views.py:
    • Explicitly handle errors using new custom exceptions.
    • Return appropriate HTTP status codes and error messages for better clarity.
  • Introduced custom exceptions in exceptions.py:
    • ExecutionDoesNotExistError

Why

  • Clarity in error handling: Users should receive specific and actionable error messages, e.g., whether an execution_id is invalid or doesn't exist in the database.
  • Better debugging: Enhanced logging and HTTP responses allow for easier identification and resolution of issues.

How

  • In serializers.py:
    • Added validation to enforce execution_id as a UUID.
    • Trimmed trailing spaces.
  • In api_deployment_views.py:
    • Ensured all exceptions from the helper are mapped to appropriate HTTP responses.
  • In exceptions.py:
    • Centralized error definitions for cleaner management.

Notes on Testing

  • Tested error scenarios:
    • Invalid execution_id format: Raises InvalidExecutionIDError.
    • Non-existent execution_id: Raises WorkflowDoesNotExistError.

Relates Issue

#1818

Screenshots

  1. Error for invalid execution_id format

    invalid_uuid_example

  2. Error for non-existent execution_id
    non_existent_uuid

  3. Trailing Spaces
    trailing_spaces

Checklist

I have read and understood the Contribution Guidelines.

@CLAassistant
Copy link

CLAassistant commented Dec 11, 2024

CLA assistant check
All committers have signed the CLA.

backend/api_v2/api_deployment_views.py Show resolved Hide resolved
backend/api_v2/api_deployment_views.py Outdated Show resolved Hide resolved
backend/workflow_manager/workflow_v2/workflow_helper.py Outdated Show resolved Hide resolved
backend/api_v2/serializers.py Outdated Show resolved Hide resolved
Copy link
Contributor

@chandrasekharan-zipstack chandrasekharan-zipstack left a comment

Choose a reason for hiding this comment

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

LGTM - make sure to resolve conflicts @pk-zipstack

backend/api_v2/serializers.py Outdated Show resolved Hide resolved
Copy link
Contributor

filepath function $$\textcolor{#23d18b}{\tt{passed}}$$ SUBTOTAL
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_logs}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_cleanup\_skip}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_client\_init}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image\_exists}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_image}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_get\_container\_run\_config\_without\_mount}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{worker/src/unstract/worker/clients/test\_docker.py}}$$ $$\textcolor{#23d18b}{\tt{test\_run\_container}}$$ $$\textcolor{#23d18b}{\tt{1}}$$ $$\textcolor{#23d18b}{\tt{1}}$$
$$\textcolor{#23d18b}{\tt{TOTAL}}$$ $$\textcolor{#23d18b}{\tt{9}}$$ $$\textcolor{#23d18b}{\tt{9}}$$

@ritwik-g ritwik-g merged commit fdb5a0c into main Dec 17, 2024
6 checks passed
@ritwik-g ritwik-g deleted the feat/improve-api-deployment-valdiation branch December 17, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants