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

Adding validations to rest APIs #226

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Jaychaware
Copy link
Collaborator

@Jaychaware Jaychaware commented Feb 3, 2025

Related Issues / Pull Requests

Description

Using Pydantic library, added validations to rest APIs

What changes are proposed in this pull request?

  • Added Pydantic validations to rest APIs.
  • Handled invalid json request pass to rest APIs.
  • Updated libraries in requirement.txt, setup.py and pyproject.toml files.

Checklist:

  • My code follows the style guidelines of this project (PEP-8 with Google-style docstrings).
  • My code modifies existing public API, or introduces new public API, and I updated or wrote docstrings that
    uses Google-style formatting and any other formatting that is supported by mkdocs and plugins this project
    uses.
  • I have commented my code.
  • My code requires documentation updates, and I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

Copy link
Collaborator

@shreyas-kulkarni09 shreyas-kulkarni09 left a comment

Choose a reason for hiding this comment

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

PR looks good to me. @rishabhsharma22 can you please check if any updated packages are not breaking any features.


# api to get mlmd file from cmf-server
@app.get("/mlmd_pull/{pipeline_name}", response_class=HTMLResponse)
async def mlmd_pull(info: Request, pipeline_name: str):
async def mlmd_pull(pipeline_name: str, request: MLMDPullRequest):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not a good practice to include body in GET request. "exec_id" id can be part of uri

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @shreyas-kulkarni09, I changed the mlmd_pull request to accept exec_id as a query parameter instead of in the JSON body. Thanks for the heads-up!

Copy link
Collaborator

@shreyas-kulkarni09 shreyas-kulkarni09 left a comment

Choose a reason for hiding this comment

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

Comments are good to have changes not a blocker. If suggested changes don't break other functionalities can be done as part of this issue or we can have separate issue for it

@rishabhsharma22
Copy link
Collaborator

PR looks good to me. I think the issue that @shreyas-kulkarni09 has raised must be addressed and we are good to merge it.

Copy link
Collaborator

@shreyas-kulkarni09 shreyas-kulkarni09 left a comment

Choose a reason for hiding this comment

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

PR looks good to me.

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.

4 participants