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

Better handling of pydantic error msgs #546

Merged
merged 6 commits into from
Oct 9, 2023
Merged

Better handling of pydantic error msgs #546

merged 6 commits into from
Oct 9, 2023

Conversation

pierrotsmnrd
Copy link
Contributor

@pierrotsmnrd pierrotsmnrd commented Aug 25, 2023

Fixes #535 (work in progress)

Description

The purpose of this PR is to present more user-friendly error messages, in case of invalid YAML specification when trying to create an env.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentaion (if required)?
  • Did you add/update relevant tests for this change (if required)?

Additional information

I consider this PR as a work in progress.

  • the unique error case currently handled, might have its human-friendly message improved
  • we might handle there other cases than the one described in the original issues
  • Does the "translation" pydantic error ➞ human need to be done somewhere else ?
  • Also : unit tests

@costrouc
Copy link
Member

Personally there are two things that have bugged me about pydantic error messages:

  • context
  • human readable messages

For context it would be nice to see the offending lines/location

For human readable messages I imagine it might be worth putting some work into the schema CondaSpecification and adding some additional validation to ensure that the messages are generally better.

"Invalid YAML. A forbidden `None` value has been encountered. "
)

raise HTTPException(status_code=400, detail=human_readable_error)
Copy link
Contributor

Choose a reason for hiding this comment

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

A more scalable way to handle this would be to have a subclass of the HTTPException class that handles the translation in the constructor.

@netlify
Copy link

netlify bot commented Sep 8, 2023

Deploy Preview for kaleidoscopic-dango-0cf31d canceled.

Name Link
🔨 Latest commit 5f078e1
🔍 Latest deploy log https://app.netlify.com/sites/kaleidoscopic-dango-0cf31d/deploys/65111649caeaed0008753136

@pierrotsmnrd pierrotsmnrd changed the title [WIP] better handling of pydantic error msgs Better handling of pydantic error msgs Sep 8, 2023
@pierrotsmnrd
Copy link
Contributor Author

I have followed @costrouc 's recommendations.

Now, the validation is done in the CondaSpecification class, and all the validation errors are raised (we use to display only one)

For instance, for a spec where the name of the env is missing, and an empty dependency exists, we have the two errors like this :
Capture d’écran 2023-09-08 à 08 12 48

We need to add a fix in the UI to handle the \n in between the two errors.

@@ -564,6 +564,8 @@ async def api_post_specification(
specification = schema.CondaSpecification.parse_obj(specification)
except yaml.error.YAMLError:
raise HTTPException(status_code=400, detail="Unable to parse. Invalid YAML")
except ValueError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

You should use a custom exception here. Something else could raise a ValueError which wouldn't have a list in its args, and that probably shouldn't be converted to an HTTPException anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator

@trallard trallard left a comment

Choose a reason for hiding this comment

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

This looks good to me, @asmeurer since you had reviewed this before, can you have another look at this and approve if applicable?

@asmeurer
Copy link
Contributor

asmeurer commented Oct 5, 2023

This looks fine. We'll probably want to refactor this if we start translating a lot more error messages, but with just a couple the way it's written now is fine.

@trallard trallard merged commit 6d66e77 into main Oct 9, 2023
13 checks passed
@trallard trallard deleted the fix535 branch October 9, 2023 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

Improve pydantic validation check error reporting on yaml specification
4 participants