-
Notifications
You must be signed in to change notification settings - Fork 9
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
build: upgrade FOCA to 0.13.0 #52
Conversation
Reviewer's Guide by SourceryThis pull request updates the FOCA library to version 0.13.0. The primary change involves updating the way the FOCA configuration is accessed within the application code and tests. Specifically, the configuration is now accessed using File-Level Changes
Tips
|
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.
Hey @jvkersch - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Avoid using
eval
for security reasons. (link)
Here's what I looked at during the review
- 🟡 General issues: 5 issues found
- 🔴 Security: 1 blocking issue
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -121,10 +121,10 @@ def __add_access_ids(data: List) -> List: | |||
Access methods metadata complete with unique access identifiers. | |||
""" | |||
id_charset = eval( | |||
current_app.config['FOCA'].endpoints['access_methods']['id_charset'] | |||
current_app.config.foca.endpoints['access_methods']['id_charset'] |
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.
🚨 issue (security): Avoid using eval
for security reasons.
Using eval
can introduce security vulnerabilities. Consider using a safer alternative like ast.literal_eval
if the input is expected to be a valid Python literal structure.
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.
Flagged as #54.
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 can address #54 right after this PR, but I prefer not mixing maintenance changes and behavioral changes.
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.
Only one minor comment. We have made some repetitive usages but we can take that up in a separate refactor as well.
Thanks @kushagra189, I've done that particular refactor in the module where there were a lot of repeated usages of that object. There was another instance where Sourcery suggested a refactor but that was not applicable (the repeated code was spread out over multiple functions). The only open issue is the use of |
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.
Thanks @jvkersch , LGTM.
* build: upgrade FOCA version
Description
Updates FOCA to the latest version, which is 0.13.0 at the time this PR was opened.
Fixes #49
Type of change
Please delete options that are not relevant.
Checklist:
Summary by Sourcery
This pull request updates the FOCA library to version 0.13.0, refactors the code to use the new configuration syntax, and adjusts the test cases accordingly to ensure compatibility and functionality.