Skip to content

Conversation

@shahdyousefak
Copy link
Contributor

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

  • I referenced the issue addressed in this PR.
  • I described the changes made and how these address the issue.
  • I described how I tested these changes.

Coding/Commit Requirements

  • I followed applicable coding standards where appropriate (e.g., PEP8)
  • I have not committed any models or other large files.

New Component Checklist (mandatory for new microservices)

  • I added an entry to docker-compose.yml and build.yml.
  • I created A CI workflow under .github/workflows.
  • I have created a README.md file that describes what the component does and what it depends on (other microservices, ML models, etc.).

OR

  • I have not added a new component in this PR.

@shahdyousefak shahdyousefak requested a review from gvzdv August 22, 2025 00:43
Copy link
Contributor

@gvzdv gvzdv left a 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
Copy link
Contributor

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
Copy link
Contributor

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:

  1. Validation errors are logged with details in the development environment by the module.
  2. Validation errors are logged with generic messages in the production environment by the module.
  3. The component doesn't receive error messages at all.

Let me know if I'm missing an obvious reason not to do that!

@shahdyousefak shahdyousefak linked an issue Aug 22, 2025 that may be closed by this pull request
@gvzdv gvzdv assigned shahdyousefak and unassigned gvzdv and jaydeepsingh25 Aug 25, 2025
Copy link
Contributor

@gvzdv gvzdv left a 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.

@shahdyousefak shahdyousefak merged commit 62e20fc into main Aug 27, 2025
42 checks passed
@shahdyousefak shahdyousefak deleted the schema-refactor branch August 27, 2025 15:48
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.

Consolidate code for json schema validation

4 participants