Skip to content

fix: Robust Recursion Handling & Enhanced Migration Tools #109

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.

Sign up for GitHub

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

Open
zerodiscount wants to merge 1 commit into
base: version-15
Choose a base branch
from
Open

fix: Robust Recursion Handling & Enhanced Migration Tools #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 71 additions & 19 deletions cloud_storage/cloud_storage/overrides/file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,11 @@ def validate(self) -> None:
PATH: frappe/core/doctype/file/file.py
METHOD: validate
"""
self.associate_files()
if self.flags.cloud_storage or self.flags.ignore_file_validate:
return

self.associate_files()

if not self.is_remote_file:
self.custom_validate()
else:
Expand Down Expand Up @@ -211,7 +213,13 @@ def associate_files(
"file_association",
add_child_file_association(attached_to_doctype, attached_to_name),
)
existing_file.save()
existing_file.flags.ignore_file_validate = True

if self.flags.ignore_links: existing_file.flags.ignore_links = True
if self.flags.ignore_permissions: existing_file.flags.ignore_permissions = True
if self.flags.ignore_validate: existing_file.flags.ignore_validate = True

existing_file.save(ignore_permissions=True)
else:
if self.file_association:
already_linked = any(
Expand Down Expand Up @@ -395,10 +403,15 @@ def has_permission(doc, ptype: str | None = None, user: str | None = None) -> bo
if doc.owner == user:
has_access = True
elif doc.attached_to_doctype and doc.attached_to_name: # type: ignore
reference_doc = frappe.get_doc(doc.attached_to_doctype, doc.attached_to_name) # type: ignore
has_access = reference_doc.has_permission()
if not has_access:
has_access = has_user_permission(doc, user)
try:
reference_doc = frappe.get_doc(doc.attached_to_doctype, doc.attached_to_name) # type: ignore
has_access = reference_doc.has_permission()
if not has_access:
has_access = has_user_permission(doc, user)
except frappe.DoesNotExistError:
# If attached document doesn't exist, check permission on the file itself
has_access = bool(frappe.has_permission(doc.doctype, ptype, user=user))

# elif True:
# Check "shared with" including parent 'folder' to allow access
# ...
Expand Down Expand Up @@ -439,11 +452,31 @@ def strip_special_chars(file_name: str) -> str:
return regex.sub("", file_name)


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)
}

Comment on lines +455 to +473
Copy link
Copy Markdown
Collaborator

@lauty95 lauty95 Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@frappe.whitelist()
def get_cloud_storage_client():
validate_config()

config: dict = frappe.conf.cloud_storage_settings
config: dict = get_cloud_storage_config()
session = Session(
aws_access_key_id=config.get("access_key"),
aws_secret_access_key=config.get("secret"),
Expand All @@ -462,7 +495,7 @@ def get_cloud_storage_client():


def validate_config() -> None:
config: dict = frappe.conf.cloud_storage_settings
config: dict = get_cloud_storage_config()

if not config:
frappe.throw(
Expand Down Expand Up @@ -560,14 +593,23 @@ def get_file_path(file: File, folder: str | None = None) -> str:
except Exception as e:
frappe.log_error(f"Custom path generator failed: {str(e)}", "Cloud Storage Path Error")

config = frappe.conf.get("cloud_storage_settings", {})
config = get_cloud_storage_config()
if config.get("use_legacy_paths", True):
return _legacy_get_file_path(file, folder)
# Verify if this is a fresh install or if we want to enforce new paths even with legacy flag
# For Hygient/Zerodiscount: We enforce strict site segregation.
pass

# Standard Logic
path = file.file_name
if folder:
return f"{folder}/{file.file_name}"
path = f"{folder}/{file.file_name}"

return file.file_name
# Enforce Site Segregation
# e.g. "site1.local/folder/filename.jpg"
if hasattr(frappe.local, "site") and frappe.local.site:
return f"{frappe.local.site}/{path}"
Comment on lines +609 to +610
Copy link
Copy Markdown
Collaborator

@lauty95 lauty95 Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return path


def _legacy_get_file_path(file: File, folder: str | None = None) -> str:
Expand Down Expand Up @@ -599,9 +641,8 @@ def get_file_content_hash(content, content_type):

@frappe.whitelist()
def write_file(file: File, remove_spaces_in_file_name: bool = True) -> File:
if not frappe.conf.cloud_storage_settings or frappe.conf.cloud_storage_settings.get(
"use_local", False
):
config = get_cloud_storage_config()
if not config or config.get("use_local", False):
file.save_file_on_filesystem()
return file

Expand All @@ -618,8 +659,14 @@ def write_file(file: File, remove_spaces_in_file_name: bool = True) -> File:

if existing_file_hashes:
file_doc: File = frappe.get_doc("File", existing_file_hashes[0])

# Propagate flags
if file.flags.ignore_links: file_doc.flags.ignore_links = True
if file.flags.ignore_permissions: file_doc.flags.ignore_permissions = True
if file.flags.ignore_validate: file_doc.flags.ignore_validate = True

file_doc.associate_files(file.attached_to_doctype, file.attached_to_name)
file_doc.save()
file_doc.save(ignore_permissions=True)
return file_doc

# if a filename-conflict is found, update the existing document with a new version instead
Expand All @@ -636,6 +683,12 @@ def write_file(file: File, remove_spaces_in_file_name: bool = True) -> File:
"content_type": file.content_type,
}
)

# Propagate flags
if file.flags.ignore_links: file_doc.flags.ignore_links = True
if file.flags.ignore_permissions: file_doc.flags.ignore_permissions = True
if file.flags.ignore_validate: file_doc.flags.ignore_validate = True

file_doc.associate_files(file.attached_to_doctype, file.attached_to_name)
file = file_doc

Expand All @@ -649,9 +702,8 @@ def write_file(file: File, remove_spaces_in_file_name: bool = True) -> File:

@frappe.whitelist()
def delete_file(file: File, **kwargs) -> File:
if not frappe.conf.cloud_storage_settings or frappe.conf.cloud_storage_settings.get(
"use_local", False
):
config = get_cloud_storage_config()
if not config or config.get("use_local", False):
file.delete_file_from_filesystem()
return file

Expand Down
39 changes: 37 additions & 2 deletions cloud_storage/migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@
get_file_path,
validate_config,
write_file,
get_cloud_storage_config,
FILE_URL,
)


Expand Down Expand Up @@ -41,7 +43,7 @@ def migrate_files(
"""
validate_config()

config = frappe.conf.get("cloud_storage_settings")
config = get_cloud_storage_config()
if config.get("use_local"):
frappe.throw(
"Cloud Storage is not enabled. Please set 'use_local' to 0 in 'cloud_storage_settings'."
Expand Down Expand Up @@ -109,6 +111,11 @@ def migrate_files(
for file_data in batch:
try:
file_doc = frappe.get_doc("File", file_data.name)
if not file_doc.file_name:
print(f"⚠️ SKIP: {file_doc.name} - Missing file_name")
stats["skipped"] += 1
continue

if not file_doc.is_private:
file_path = frappe.get_site_path("public", file_doc.file_url.lstrip("/"))
else:
Expand Down Expand Up @@ -136,13 +143,39 @@ def migrate_files(
print(f" Attached to: {attached_to}")
print(f" Local path: {file_doc.file_url}")

if not dry_run:
client = get_cloud_storage_client()
config = get_cloud_storage_config()
expected_s3_key = get_file_path(file_doc, config.get("folder"))

try:
client.head_object(Bucket=client.bucket, Key=expected_s3_key)
# If we reach here, file exists in S3. Sync DB and Skip Upload.
file_doc.db_set("s3_key", expected_s3_key)
file_doc.db_set("file_url", FILE_URL.format(path=expected_s3_key))
print(f" ✅ Synced Record (Found in Cloud): {expected_s3_key}")
stats["migrated"] += 1

if remove_local and os.path.exists(file_path):
os.remove(file_path)
print(" 🗑️ Deleted local file (Synced)")

continue
except ClientError:
# File not found in S3, proceed with upload
pass

if not dry_run:
with open(file_path, "rb") as f:
file_content = f.read()

file_doc.content = file_content
content_type, _ = mimetypes.guess_type(file_doc.file_name)
file_doc.content_type = content_type or "application/octet-stream"

file_doc.flags.ignore_links = True
file_doc.flags.ignore_permissions = True
file_doc.flags.ignore_validate = True

new_file = write_file(file_doc)
new_file.reload()
Expand All @@ -164,7 +197,9 @@ def migrate_files(
print()

except Exception as e:
import traceback
print(f"❌ FAILED: {file_data.name} - {str(e)}")
traceback.print_exc()
frappe.log_error(
title=f"Cloud Storage Migration Failed: {file_data.name}", message=frappe.get_traceback()
)
Expand Down Expand Up @@ -194,7 +229,7 @@ def migrate_files(
def migrate_paths(dry_run=False, limit=None, batch_size=100):
"""Migrate files from legacy paths to new path strategy."""
client = get_cloud_storage_client()
config = frappe.conf.get("cloud_storage_settings", {})
config = get_cloud_storage_config()

if config.get("use_legacy_paths", True):
print("⚠️ Legacy paths are still enabled in cloud_storage_settings.")
Expand Down
Loading