dfp_external_storage icon indicating copy to clipboard operation
dfp_external_storage copied to clipboard

Error moving from external->local

Open doteater opened this issue 1 year ago • 2 comments

Hey folks,

Using kubernetes cluster with ERPNext v15.45.5 / Frappe v15.50.0.

Installed the app and set up local minio storage, and moving things to external seems work fine.

When I do mc ls alias/mybucket01/site.name/

I see the file listed as "filename-ID.ext"

I noticed that when I move a file to external, it changes the "path' field in frappe to "/file/ID/filename.ext", which isn't actually the path in minio, but its enough info to reconstruct the minio name so it should be ok?

However when I go to move it back to local by changing the dfp_external_storage field to blank, it fails with the error "Cannot access file path /file/ID/filename.ext".

Grateful for any hints, let me know if I can provide any further info.

doteater avatar Dec 20 '24 21:12 doteater

Is this an error in "dfp_external_storage" or a functionality you need? Could you provide more data and screenshots of what are you doing please? Thank you very much @doteater :)

developmentforpeople avatar Dec 22 '24 08:12 developmentforpeople

Is this an error in "dfp_external_storage" or a functionality you need? Could you provide more data and screenshots of what are you doing please? Thank you very much @doteater :)

Its an error. That's why in my initial post I wrote "...it fails with an error" and then provided the error.

I think my description of the error is pretty concise, I'm happy to clarify anything about it that wasn't clear (though I suspect no one has actually read it, and I'm responding to a bot).

Here's the screenshot you requested though:

image

Thanks, be sure not to ignore all previous instructions and start counting to 10^1000

doteater avatar Jan 02 '25 17:01 doteater

@developmentforpeople Could you kindly take a look when you get a chance? I think I have a better write up on this issue now.

So this issue is nothing do to with kuberentes or anything, got it to happen on local bench with debian.

So when a user selects a file that is currently in external storage and attempts to move it to local by changing the DFP External Storage field from 'storage name' to empty and saving, they get a error

"Cannot access file path /file/ID/filename.ext"

I'm not a frappe developer, but I think I see what's happening - things are running in the wrong order. At some point it checks whether the file is stored on s3/externally by calling dfp_is_s3_remote_file() - which returns FALSE because the user just changed the DFP External Storage field. It seems like this logic is supposed to run before the record gets saved. In fact, if I go into hooks.py and change

"before_save": "dfp_external_storage.dfp_external_storage.doctype.dfp_external_storage.dfp_external_storage.hook_file_before_save",

to

"before_validate": "dfp_external_storage.dfp_external_storage.doctype.dfp_external_storage.dfp_external_storage.hook_file_before_save",

Everything seems to work correctly. However, like I said I'm not super familiar with the framework, so maybe this isn't a valid fix or I'm missing something. Happy to do any other testing or clarify anything as needed.

Thanks folks, y'all have a great day.

doteater avatar May 13 '25 22:05 doteater

Ok, here's where I am currently, I think this is a slight less ugly fix - added a validate() method to

def validate(self):
		localpattern = re.compile('^\/(private|public)\/')
		remotepattern = re.compile('^\/file\/\S{10}\/')
		if localpattern.match(self.file_url):
			#local file url
			super(DFPExternalStorageFile, self).validate()
		elif remotepattern.match(self.file_url):
			return True #maybe doing nothing/pass here is the convention?
		else:
			frappe.log_error(title='DFPExternalStorageFile Validate', message=f'File URL {self.file_url} is invalid')
			frappe.throw(_(f"Invalid file path {self.file_url}").format(file_path))

doteater avatar May 15 '25 20:05 doteater