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

Validate GraphAddRequest using Pydantic validators #851

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

nazywam
Copy link
Contributor

@nazywam nazywam commented Oct 24, 2023

Hey, while playing around with the API I've noticed that if you try to add a graph link with malformed source/target fields (like "source": "abc" the server responds with an ugly exception:

  File "/root/.cache/pypoetry/virtualenvs/yeti-9TtSrW0h-py3.10/lib/python3.10/site-packages/fastapi/routing.py", line 190, in run_endpoint_function
    return await dependant.call(**values)
  File "/app/core/web/apiv2/graph.py", line 88, in add
    source_type, source_id = request.source.split("/")
ValueError: not enough values to unpack (expected 2, got 1)

How do you feel about validating the incoming fields using pydantic validators?

This is how the response looks in comparison to the previous one:
image

Probably a similar approach can be used for other incoming models as well.

@tomchop
Copy link
Collaborator

tomchop commented Oct 24, 2023

Amazing, thanks! Do you know if we can make source and target be of a custom type with a validator, instead of setting a validator on the string? I'm thinking that I'd prefer having this sit closer to the ArangoDB model (given that type/id is an arangoDB implementation detail)

@nazywam
Copy link
Contributor Author

nazywam commented Oct 24, 2023

Yeah, I think it should be possible, it'd probably look something like this -> https://docs.pydantic.dev/latest/concepts/types/#custom-types

I can try modyfing the PR if you'd like

@tomchop
Copy link
Collaborator

tomchop commented Oct 25, 2023

Changing this across the whole codebase might have other unintended side-effects. Let's merge this change as it is now. I opened #852 to track the wider changes. Thank you!!

@tomchop tomchop merged commit 47d45fa into yeti-platform:fastapi Oct 25, 2023
@tomchop tomchop self-requested a review October 25, 2023 07:31
@nazywam nazywam deleted the feature/graph-add-validation branch October 25, 2023 07:47
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.

2 participants