Conversation
|
I have read the CLA Document and I hereby sign the CLA 1 out of 2 committers have signed the CLA. |
There was a problem hiding this comment.
Pull request overview
This pull request addresses security vulnerabilities by updating dependencies and improving thread safety in the database layer.
Changes:
- Upgraded urllib3 (2.0.6 → 2.6.3) and Werkzeug (3.0.6 → 3.1.5) to address security vulnerabilities
- Updated PIP (22.2.0 → 25.3.0) and SETUPTOOLS (70.0.0 → 80.9.0) in the Alpine Dockerfile
- Refactored DB class to store connections instead of cursors, added threading locks, and implemented cursor context managers for better resource management
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| requirements.txt | Updated urllib3 and Werkzeug to newer versions to resolve security vulnerabilities |
| alpine/Dockerfile | Updated PIP and SETUPTOOLS versions to their latest releases |
| src/db.py | Enhanced thread safety with locks, changed to connection-based architecture, and added cursor context managers |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
|
|
||
| class DB: | ||
| _lock = threading.Lock() |
There was a problem hiding this comment.
The _lock is a class-level attribute shared across all DB instances. This means all database operations across different database files will compete for the same lock, which could cause unnecessary serialization and performance degradation. Consider using an instance-level lock (self._lock = threading.Lock() in init) if each DB instance operates on a separate database file.
| with DB._lock: | ||
| log.debug("SQL command: " + sql.replace('?', '%s') % args) | ||
| cursor.execute(sql, args) |
There was a problem hiding this comment.
The threading lock is being used only around the logging and execute operations in __log_and_execute, but this may not provide adequate thread safety. The lock should protect the entire database operation sequence, not just the execute call. Consider that cursor operations might need protection at a higher level, or use connection-level locks instead of a class-level lock since each DB instance has its own connection.
| if login: | ||
| cursor = DB.__create_connection(self.__dbfile).cursor() | ||
| else: | ||
| cursor = self.__cursor | ||
|
|
||
| DB.__log_and_execute(cursor, query, params) | ||
| cursor = self.__conn.cursor() | ||
| with cursor: | ||
| DB.__log_and_execute(cursor, query, params) |
There was a problem hiding this comment.
When login=True, a new connection is created but never explicitly closed, which could lead to resource leaks. The cursor context manager only closes the cursor, not the underlying connection. Consider storing the connection and ensuring it's properly closed, or restructure to reuse the existing connection.
| if login: | ||
| cursor = DB.__create_connection(self.__dbfile).cursor() | ||
| else: | ||
| cursor = self.__cursor | ||
|
|
||
| DB.__log_and_execute(cursor, query, params) | ||
| return cursor.fetchall() | ||
| cursor = self.__conn.cursor() | ||
| with cursor: | ||
| DB.__log_and_execute(cursor, query, params) | ||
| return cursor.fetchall() |
There was a problem hiding this comment.
When login=True, a new connection is created but never explicitly closed, which could lead to resource leaks. The cursor context manager only closes the cursor, not the underlying connection. Consider storing the connection and ensuring it's properly closed, or restructure to reuse the existing connection.
|
@copilot open a new pull request to apply changes based on the comments in this thread |
|
I have read the CLA Document and I hereby sign the CLA |
Co-authored-by: borislavr <noreply@github.com> Co-authored-by: Philipp Bouzid <ph.bouzid03@gmail.com>
Co-authored-by: NetcrackerCLPLCI <noreply@github.com>
Co-authored-by: borislavr <noreply@github.com> Co-authored-by: Philipp Bouzid <ph.bouzid03@gmail.com>
…ependencies (#48) Co-authored-by: borislavr <noreply@github.com>
…racker/qubership-backup-daemon into fix/resolve_vulnerabilities
What type of PR is this? (check all applicable)
Description
TDB
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Breaking Change checklist
If your PR includes any deployment or processing changes, please utilize this checklist:
Added/updated tests?
have not been included
[optional] Are there any things to highlight or double check?
[optional] What gif best describes this PR or how it makes you feel?