-
Notifications
You must be signed in to change notification settings - Fork 7
Schema refactor #1137
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
Schema refactor #1137
Conversation
…ding 3 reusable validator objects from self._compile()
gvzdv
left a comment
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.
Great refactoring work! Let's keep making the code cleaner and more beautiful.
A couple of comments above + please fill out the PR body to log the changes and how they were tested.
|
|
||
| COPY /schemas /app/schemas | ||
| COPY /config /app/config | ||
| COPY /utils /app/utils |
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.
Does it make sense to copy only /utils/validation to the preprocessors that don't use other utilities to reduce the Docker image size?
| return validated | ||
| ok, _ = VALIDATOR.check_request(content) | ||
| if not ok: | ||
| return jsonify("Invalid Request JSON format"), 400 |
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.
We never access the error message, so I'm not sure we need it at all.
Does it make sense to modify the check_* functions to return the Boolean values (instead of tuples)?
Example:
def check_request(self, data):
"""
Validate final response envelope; return (ok, err).
Logs on failure via validate_response().
"""
try:
self.validate_request(data)
return True
except jsonschema.exceptions.ValidationError:
return False
Then we can simplify the check in the preprocessors to
if not VALIDATOR.check_request(content):
return jsonify("Invalid Request JSON format"), 400
Results:
- Validation errors are logged with details in the development environment by the module.
- Validation errors are logged with generic messages in the production environment by the module.
- The component doesn't receive error messages at all.
Let me know if I'm missing an obvious reason not to do that!
gvzdv
left a comment
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.
As discussed, these items are deemed non-critical. PR approved.
6f7648b to
064b774
Compare
Please ensure you've followed the checklist and provide all the required information before requesting a review.
If you do not have everything applicable to your PR, it will not be reviewed!
If you don't know what something is or if it applies to you, ask!
Please note that PRs from external contributors who have not agreed to our Contributor License Agreement will not be considered.
To accept it, include
I agree to the [current Contributor License Agreement](/CLA.md)in this pull request.Don't delete below this line.
Required Information
Coding/Commit Requirements
New Component Checklist (mandatory for new microservices)
docker-compose.ymlandbuild.yml..github/workflows.README.mdfile that describes what the component does and what it depends on (other microservices, ML models, etc.).OR