fix: Robust Recursion Handling & Enhanced Migration Tools#109
Open
zerodiscount wants to merge 1 commit intoagritheory:version-15from
Open
fix: Robust Recursion Handling & Enhanced Migration Tools#109zerodiscount wants to merge 1 commit intoagritheory:version-15from
zerodiscount wants to merge 1 commit intoagritheory:version-15from
Conversation
This commit introduces several stability improvements and tools for the Cloud Storage app, developed during a large-scale enterprise migration.
1. **Fix Recursion Error**:
* Addressed a RecursionError in CloudStorageFile.validate. The recursive loop occurred when associate_files() called save(), re-triggering validation. This fix checks ignore_file_validate *before* calling association logic.
2. **Robust Migration Tools**:
* Updated migrate-files-to-cloud-storage:
* **Smart Sync**: Checks if file already exists in S3 (head_object) before uploading. If found, it syncs the DB record (s3_key) and skips upload.
* **Validation Bypass**: Added ignore_links, ignore_permissions, and ignore_validate flags to file_doc.
3. **File Logic Hardening**:
* Updated write_file and associate_files in overrides/file.py to correctly propagate validation flags when updating *existing* file records.
* Patched has_permission to gracefully handle DoesNotExistError.
lauty95
reviewed
Jan 26, 2026
Comment on lines
+455
to
+473
| def get_cloud_storage_config() -> dict: | ||
| config = frappe.conf.get("cloud_storage_settings", {}) | ||
|
|
||
| # If nested config is found and seems populated with at least access_key, use it. | ||
| if config and config.get("access_key"): | ||
| return config | ||
|
|
||
| # Otherwise, build from top-level standard keys | ||
| return { | ||
| "access_key": frappe.conf.get("s3_access_key"), | ||
| "secret": frappe.conf.get("s3_secret_key"), | ||
| "bucket": frappe.conf.get("s3_bucket"), | ||
| "region": frappe.conf.get("region"), | ||
| "endpoint_url": frappe.conf.get("endpoint_url"), | ||
| "folder": frappe.conf.get("s3_folder"), | ||
| "use_local": frappe.conf.get("use_local"), | ||
| "use_legacy_paths": frappe.conf.get("use_legacy_paths", True) | ||
| } | ||
|
|
Collaborator
There was a problem hiding this comment.
Although this function is a good idea, I recommend using the same keys documented in the configuration guide: https://github.com/agritheory/cloud_storage/blob/version-15/docs/configuration.md
This would maintain consistency with the official documentation and avoid confusion for users following the setup instructions.
Comment on lines
+609
to
+610
| if hasattr(frappe.local, "site") and frappe.local.site: | ||
| return f"{frappe.local.site}/{path}" |
Collaborator
There was a problem hiding this comment.
This change is not necessary. If you need a custom path, you can configure it in site_config.json:
{
"cloud_storage_settings": {
"folder": "site1.local/folder",
...
}
}This approach is more flexible and doesn't force a specific path structure on all installations.
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
2 participants
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.
This PR introduces several stability improvements and tools for the Cloud Storage app, developed during a large-scale enterprise migration.
Key Changes:
Fix Recursion Error:
Robust Migration Tools:
File Logic Hardening:
These changes make the app significantly more robust for real-world production environments and bulk migrations.