-
Notifications
You must be signed in to change notification settings - Fork 456
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
Fix request model validation #3245
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an invalid UUID was being passed for a equals: filter, we threw a ValueError. This did not match the behaviour when using e.g. a oneof:[] filter and leads to additional complexity in the frontend, which is why we removed this error. The filter with an invalid ID will now simply return an empty list.
I would propose approaching this the other way around. If we throw an error when people use an invalid UUID on a field which is clearly UUID, it is much more helpful to the user (and to the frontend) IMO. So, instead of removing the check from equals:
, I would propose adding it to oneof:
as well and keep things consistent.
I could make the same argument for date filters before the year 2020 though, as ZenML didn't exist yet and therefore no pipeline run can be created before that. Or any name that contains invalid characters. Or a negative integer for a version number. Especially for the filter implementation in the frontend, it's very weird if the user switches the operator from |
I see your point. However, I have to say, throwing an error if someone enters a name with an invalid character made sense to me intuitively. 😄 I guess, the main use case I was trying to avoid was this: Back in the day (not sure if this is still the case), some of our UUID filters would work with the I agree with you completely regarding the switch though from the frontend perspective. Maybe @stefannica and @AlexejPenner can chime in here as well. Regardless, I don't think this is a critical issue. I will approve for now, feel free to merge as you see fit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦭
I like the idea of throwing errors for dates before 2020 - maybe a 1985: Time travel not yet invented Error
?
Describe changes
This PR contains two changes:
make_dependable
function that allows us to use filter models attributes as query parameters in FastAPI endpoints had a bug where it would throw a500
HTTP error instead of the intended422
, because there were some objects that are not JSON serializable in theValidationError.errors()
list that we passed on to theHTTPError
. This PR fixes that by instead using our helper function to compute the error detail.equals:<ID>
filter, we threw a ValueError. This did not match the behaviour when using e.g. aoneof:[<ID>]
filter and leads to additional complexity in the frontend, which is why we removed this error. The filter with an invalid ID will now simply return an empty list.Pre-requisites
Please ensure you have done the following:
develop
and the open PR is targetingdevelop
. If your branch wasn't based on develop read Contribution guide on rebasing branch to develop.Types of changes