Open
Conversation
- mmc.database.sqlite_helper.SqliteHelper is a generic object used as parent object - BackupSettings and BackupServer are child objects of sqliteHelper. Use this mapping creation example (cherry picked from commit 7c846d4)
This is to replace the session used as property because the @propery will no longer be maintained in python3.12 (cherry picked from commit 9316950)
(cherry picked from commit 3f5e561)
… du jid. Ce qui empeche de trouver 1 machine si c'est 1 relay server. (cherry picked from commit 12f7b7d)
… repeter dans les list. ce qui diminue la taille des messages. (cherry picked from commit 920f106)
…chine meme reseau que urbackup.
exemple appel xmlrpc. transfert
xmppmaster.backup_restore('amu-win-5.ynr@pulse/52540c32cff00', backup de la machine
'amu-win-5.ynr@pulse/52540c32cff00', ver machine ou l'on restaure
'/media/BACKUP/urbackup/amu-win-6/240916-1727', le backup conserne
['C_0/Users/pulseuser'], list de repertoire a recupere.
['Users/desktop.ini', 'C_0/Windows/SysWOW64/rsync.exe']) list de fichier a recuperer
(cherry picked from commit 333d428)
…s 1 machine LAN ou WAN. Cette fonction est appele lorsque les api urbackup ne permet pas de faire des restaurations de machine differente.
les fonctions xmlrpc backend utilisent les plugins substitut master pour generer les messages de transferts. elle appelle les plugins de transfert de fichier sur substitut master. plugin backup_restore (sur substitut master) pour retaure fichier via scp pour les machine local. plugin send_file_xmpp (sur substitut master) pour les transferts via xmpp toutes machines avec (agent machine + pluging recombine_file) Principe du transfert de fichiers via XMPP : 1. Initiation du transfert via le backend XML-RPC : Le client, généralement via un serveur XML-RPC, envoie une demande de transfert de fichier au substitut master en appelant le plugin send_file_xmpp. Ce plugin a pour rôle de préparer le fichier ou le répertoire à transférer en créant une archive ZIP. 2. Préparation de l'archive zip pour le transfert : le repertoire de travail est /var/lib/pulse2/zip_transfert/ Le plugin crée une archive ZIP, que ce soit pour un fichier unique, un répertoire, ou plusieurs fichiers, selon la demande. Une fois l'archive prête, celle-ci est découpée en plusieurs segments (ou tronçons), chaque segment étant numéroté, à partir de 1. Un fichier manifeste est également généré pour décrire la structure de l'archive ou des fichiers à transférer. Par convention, ce fichier de manifeste porte l'index 0. Les segments de l'archive, accompagnés de leur numéro d’index, sont ensuite stockés dans un répertoire de travail dédié sur le substitut master. 3. Transmission des segments : Un plugin nommé load_send_segment_file, présent sur le substitut master, surveille le répertoire de travail. Lorsqu'il détecte des transferts prêts à être envoyés, il commence à envoyer les segments à la machine de destination via des messages XMPP. Chaque message XMPP contient un segment du fichier ainsi que son numéro d’index, garantissant que les segments seront reçus et réassemblés dans le bon ordre. 4. Réception et recomposition des segments : Sur la machine de destination, le plugin recombine_file reçoit les segments. Il collecte les segments dans un répertoire de travail et, une fois tous les segments reçus, il reconstitue l'archive ou le fichier d’origine. Après cette recomposition, le plugin vérifie l'intégrité du fichier en comparant son empreinte MD5 avec celle de l'original. Si les empreintes correspondent, l'archive est jugée valide et intacte. 5. Installation du fichier : Une fois le fichier ou le répertoire reconstitué avec succès, il est décompressé sur la machine de destination, ce qui constitue le processus d'installation. Si un fichier portant le même nom existe déjà, il sera remplacé par défaut. Dans de futures versions, une option pourrait être ajoutée pour permettre à l'utilisateur de choisir s'il souhaite ou non remplacer un fichier existant. (cherry picked from commit 2c2b551)
- Add pagination for the backups on a machine - Take off the sqlalchemy mapping on sqlite objects, due to some threads issues (cherry picked from commit 396bce3)
The selected files and folders are stored in a basket. Files and folders are stored in 2 differents list (cherry picked from commit 32eb313)
Reviewer's Guide by SourceryThis pull request introduces a comprehensive backup and restore feature, enhancing the system's data protection capabilities. It includes functionalities for extracting file paths, managing file transfers over LAN and WAN, and integrating with the web interface for user interaction. The implementation involves new functions for path manipulation, network detection, and XMPP-based file transfer, along with updates to the database schema and web UI components. Updated class diagram for FileManagerclassDiagram
class FileManager {
-base_path: str
-file_list: list
-exclude_directories: list
+__init__(base_path: str = None)
+calculate_hash(file_path: str) : str
+add_fichier(path_fichier: str) : void
+add_tout_les_fichiers_directory(directory: str) : void
+get_liste() : list
+supprimer_fichier(path_fichier: str) : void
+supprimer_tout_les_fichiers_directory(directory: str) : void
+ajouter_exclude_directory(directory: str) : void
+supprimer_exclude_directory(directory: str) : void
}
note for FileManager "Manages files and directories, calculates hashes, and handles exclusions."
Updated class diagram for BackupServerclassDiagram
class BackupServer {
-path: str
-name: str
-is_activated: bool
-engine: Engine
-metadata: MetaData
+__init__()
+activate()
+_session(func)
+get_backups(session: Session, clientid: str, start: int = 0, limit: int = -1, filter: str = "") : dict
}
note for BackupServer "Manages the backup server database connection and queries."
Updated class diagram for BackupSettingsclassDiagram
class BackupSettings {
-path: str
-name: str
-is_activated: bool
-engine: Engine
-metadata: MetaData
+__init__()
+activate()
+_session(func)
+get_setting(session: Session, key: str) : str
}
note for BackupSettings "Manages the backup settings database connection and queries."
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @neoclust - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a try-except block around the call to
send_message_jsonto handle potential exceptions during message sending. - The
backup_restorefunction has a lot of code - consider breaking it down into smaller, more manageable functions.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1612
to
+1615
| # pas de jid pour la machine source. | ||
| # normalement la sauvegarde est sur le serveur urbackup. | ||
| # cela n'est pas fatal pour la restoration | ||
| if jid_from is None and machine_dest_backup is None: |
There was a problem hiding this comment.
issue (bug_risk): Undefined variable: machine_dest_backup appears to be used without prior definition.
Verify whether this variable should be 'machine_dest' or another intended value, and update accordingly to avoid runtime errors.
| return network_list | ||
|
|
||
| @DatabaseHelper._sessionm | ||
| def get_machines_summary_list(self, session, filter=""): |
There was a problem hiding this comment.
🚨 suggestion (security): SQL query uses direct string interpolation for the filter.
To reduce any potential risk, consider parameterizing the query instead of concatenating the filter variable into the SQL statement.
Suggested implementation:
params = {}
if filter != "":
sql += " AND hostname like :filter"
params['filter'] = f"%{filter}%" query = session.execute(sql, params)
Comment on lines
+70
to
+74
| result["datas"].append({ | ||
| "id":element.id, | ||
| "client_id":element.clientid, | ||
| "path":element.path, | ||
| "full": element.incremental, |
There was a problem hiding this comment.
nitpick: Confusing naming: assigning 'element.incremental' to the key 'full'.
Clarify whether this field indicates a full or incremental backup—for better readability it might be worth renaming the key or variable to accurately reflect its meaning.
| @@ -1191,6 +1198,568 @@ def get_pkg_path(login, pkgName): | |||
| def xmpp_getPackageDetail(pid_package): | |||
There was a problem hiding this comment.
issue (complexity): Consider extracting the nested helper functions within backup_restore into a separate module to improve code organization and testability.
Consider extracting the many nested helper functions inside `backup_restore` into separate, module‐level functions (or even a dedicated utility module). For example, you could move functions like `get_ip_and_netmask_linux`, `decode_and_decompress_key`, and similar into a helper file. This reduces the nesting complexity and makes unit testing of each helper function easier.
For instance, create a new file (e.g., `backup_helpers.py`):
```python
# backup_helpers.py
import socket, psutil, ipaddress, base64, zlib
def get_ip_and_netmask_linux(exclude_local=True):
ip_netmask_list = []
for interface, addrs in psutil.net_if_addrs().items():
for addr in addrs:
if addr.family == socket.AF_INET:
ip = addr.address
netmask = addr.netmask
if exclude_local:
ip_obj = ipaddress.ip_address(ip)
if ip_obj.is_loopback or ip_obj.is_link_local:
continue
ip_netmask_list.append((ip, netmask))
return ip_netmask_list
def decode_and_decompress_key(encoded_key):
compressed_key = base64.b64decode(encoded_key)
return zlib.decompress(compressed_key).decode('utf-8')
def compress_and_encode_key(key_content):
compressed_key = zlib.compress(key_content.encode('utf-8'))
return base64.b64encode(compressed_key).decode('utf-8')
# Add additional helper functions as needed.Then, in your backup_restore module, import and use these helpers:
from backup_helpers import get_ip_and_netmask_linux, decode_and_decompress_key, compress_and_encode_key
def backup_restore(src_machine, dest_machine, base_path, directorylist, filelist):
# ... existing initial code ...
ip_list = get_ip_and_netmask_linux()
# ... rest of function using imported helpers ...Following this pattern for the other nested helper functions will make backup_restore easier to follow and maintain without changing its functionality.
|
|
||
|
|
||
|
|
||
| class FileManager: |
There was a problem hiding this comment.
issue (complexity): Consider creating a private helper method to resolve paths, reducing redundancy in path manipulations throughout the class methods and improving maintainability
You can simplify the repeated path joins by extracting a helper method. For example, add a private _resolve_path method to reduce duplication. Then update your methods accordingly:
class FileManager:
def __init__(self, base_path=None):
# ... existing init code ...
self.base_path = base_path
self.file_list = []
self.exclude_directories = []
def _resolve_path(self, relative_path):
return os.path.join(self.base_path, relative_path)
def calculate_hash(self, file_path):
hash_md5 = hashlib.md5()
with open(file_path, "rb") as f:
for chunk in iter(lambda: f.read(4096), b""):
hash_md5.update(chunk)
return hash_md5.hexdigest()
def add_fichier(self, path_fichier):
abs_path = self._resolve_path(path_fichier)
if not any(f[0] == path_fichier for f in self.file_list):
md5 = self.calculate_hash(abs_path)
self.file_list.append([path_fichier, md5])
def add_tout_les_fichiers_directory(self, directory):
abs_directory = self._resolve_path(directory)
for root, dirs, files in os.walk(abs_directory):
if any(exclude in root for exclude in self.exclude_directories):
continue
for file in files:
file_path = os.path.join(root, file)
relative_path = os.path.relpath(file_path, self.base_path)
self.add_fichier(relative_path)
# Update other methods similarly; for instance:
def supprimer_tout_les_fichiers_directory(self, directory):
abs_directory = self._resolve_path(directory)
self.file_list = [f for f in self.file_list if not self._resolve_path(f[0]).startswith(abs_directory)]By centralizing path resolution, you reduce repeated code and make future changes easier while keeping all functionality intact.
Comment on lines
+44
to
+49
| for net2 in cidr_list2: | ||
| # Vérifier si les réseaux se chevauchent | ||
| if net1.overlaps(net2): | ||
| # Ajouter l'IP commune à la liste | ||
| common_addresses.append(str(net1.network_address)) | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Replace a for append loop with list extend (for-append-to-extend)
Suggested change
| for net2 in cidr_list2: | |
| # Vérifier si les réseaux se chevauchent | |
| if net1.overlaps(net2): | |
| # Ajouter l'IP commune à la liste | |
| common_addresses.append(str(net1.network_address)) | |
| common_addresses.extend( | |
| str(net1.network_address) | |
| for net2 in cidr_list2 | |
| if net1.overlaps(net2) | |
| ) |
|
|
||
| return common_addresses | ||
|
|
||
| def get_CIDR_ipv4_addresses(exclude_localhost=True): |
There was a problem hiding this comment.
issue (code-quality): Low code quality found in get_CIDR_ipv4_addresses - 17% (low-code-quality)
Explanation
The quality score for this function is below the quality threshold of 25%.This score is a combination of the method length, cognitive complexity and working memory.
How can you solve this?
It might be worth refactoring this function to make it shorter and more readable.
- Reduce the function length by extracting pieces of functionality out into
their own functions. This is the most important thing you can do - ideally a
function should be less than 10 lines. - Reduce nesting, perhaps by introducing guard clauses to return early.
- Ensure that variables are tightly scoped, so that code using related concepts
sits together within the function rather than being scattered.
Comment on lines
+2472
to
+2476
| if platform.system() == "Windows": | ||
| base_path = "C:\\" | ||
| else: | ||
| base_path = "/" | ||
|
|
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
Suggested change
| if platform.system() == "Windows": | |
| base_path = "C:\\" | |
| else: | |
| base_path = "/" | |
| base_path = "C:\\" if platform.system() == "Windows" else "/" |
| :param path_fichier: Chemin relatif du fichier. | ||
| """ | ||
| abs_path = os.path.join(self.base_path, path_fichier) | ||
| if not any(f[0] == path_fichier for f in self.file_list): |
There was a problem hiding this comment.
suggestion (code-quality): Invert any/all to simplify comparisons (invert-any-all)
Suggested change
| if not any(f[0] == path_fichier for f in self.file_list): | |
| if all(f[0] != path_fichier for f in self.file_list): |
Comment on lines
+134
to
+137
| if query is not None: | ||
| return query.value | ||
| else: | ||
| return "" No newline at end of file |
There was a problem hiding this comment.
suggestion (code-quality): Replace if statement with if expression (assign-if-exp)
Suggested change
| if query is not None: | |
| return query.value | |
| else: | |
| return "" | |
| return query.value if query is not None else "" |
(cherry picked from commit 3de5f43)
- If no machine is selected, the validate button is disabled. - Delete the destination path input and js function (no use) - Hide files table if empty - hide folders table if empty
…r simple restore After a simple restore, the redirection lost some infos. So if we add some element on the basket, the full name is wrong.
If we do not provide a name for the profile, we now, do not enable the "Create profile" button. Before this the message was "Profile already exists with this name"
(cherry picked from commit 275b2bf)
There where a confusion on the column name. The real column content is a date (not a datetime or even a time).
…empty When restoring several files/folders, instead of printing an empty message, print a default message when the return status is OK but the message is empty.
- Add translation for some strings - remove some warnings
Change the table header "Status" name to "Type of backup" on default page
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.