Kani999 / netbox-attachments

Plugin to manage attachments for any model
Apache License 2.0
55 stars 4 forks source link

Issue with naming of files #48

Closed olofvndrhr closed 8 months ago

olofvndrhr commented 9 months ago

Hello, i currently face an "issue" with this plugin about the naming of the uploaded files.

The current function which decides that is: attachment_upload in utils.py. https://github.com/olofvndrhr/netbox-attachments/blob/f67ef4aadb031f38cbc2d2d7f62164dc0e7133bd/netbox_attachments/utils.py#L1C3-L1C3

The tests were done with the following snippet (to test it locally)

class NetBoxAttachment:
    def __init__(self):
        self.name = "" # different on tests
        self.content_type = Content()
        self.object_id = 42

class Content:
    def __init__(self):
        self.name = "virtual_machine"

a = "test.zip"
b = "test.tar.gz"
c = "changelog"
x = Test()

The current implementation produces the following results if no custom name was given

print(attachment_upload(x, a))
print(attachment_upload(x, b))
print(attachment_upload(x, c))
-->
netbox-attachments/virtual_machine_42_test.zip
netbox-attachments/virtual_machine_42_test.tar.gz
netbox-attachments/virtual_machine_42_changelog

but if you specify a filename, it gets weird with files without a suffix.

self.name = "testfile" # set in the NetBoxAttachment class

print(attachment_upload(x, a))
print(attachment_upload(x, b))
print(attachment_upload(x, c))
-->
netbox-attachments/virtual_machine_42_testfile.zip
netbox-attachments/virtual_machine_42_testfile.gz
netbox-attachments/virtual_machine_42_testfile.changelog

As you see, the original name gets added as a suffix to the new name. This is because on line:

extension = filename.rsplit(".")[-1].lower()

It does not check if the list has multiple elements (name with suffix) and just returns the filename on files without a suffix. Also the instance name if/elif statement has no effect, because the "if" will always trigger.

    if instance.name:
        filename = ".".join([instance.name, extension])
    elif instance.name:
        filename = instance.name

In my eyes it also makes no sense to add the content type and id to the filename, as when you want to download your file, the filename is not what you uploaded.

I came up with 2 fixes for it:

Fix 1 Change the attachment_upload function to use "nested" files. With this approach you always get your desired filename and have no conflicts. The only downside is, i am not sure if sub-directories work in netbox plugins. But i can test it.

I can contribute the PR for this as i already made it localy and can push it to my fork. FYI the function would be:

from pathlib import Path

def attachment_upload(instance, filename: str) -> str:
    """
    Return a path for uploading file attachments.
    """
    base_path = Path("netbox-attachments")
    # instance path example: netbox-attachments/virtual_machine/42
    instance_path = base_path / instance.content_type.name / str(instance.object_id)
    filename_suffix = Path(filename).suffix

    # rename the file to the provided name, if any. attempt to preserve the file extension.
    if instance.name and Path(instance.name).suffix == filename_suffix:
        # provided name already has same extension
        filepath = instance_path / instance.name
    elif instance.name:
        # no extension in name. adding it
        filepath = instance_path / (instance.name + filename_suffix)
    else:
        # no name provided. using default filename
        filepath = instance_path / filename

    return str(filepath)

This would produces files/paths like: netbox-attachments/virtual_machine/42/test.txt We use pathlib to save some logic on the suffixes. Its in the standard lib since python 3.6. (Netbox requires 3.8)

Fix 2 The desired name gets saved in the DB. The current file naming scheme stays, but on the View and on the download, its the "desired" name.

Example: 1) Upload file test.txt (no custom name) 2) Gets saved on disk as netbox-attachments/virtual_machine_42_test.txt 3) Gets saved on DB as test.txt

-> On download and view its test.txt

But for this fix i dont have any code prepared yet.


If you think this makes no sense, dont worry. I will just use my own fork for now.

Best regards

Kani999 commented 9 months ago

Thank you, @olofvndrhr, for opening this issue. I will review it in the coming year. Unfortunately, I'm unable to address it immediately. I welcome any pull requests related to this issue

olofvndrhr commented 9 months ago

PR for Fix 1 is now open

49

Kani999 commented 8 months ago

@olofvndrhr should be fixed in version 3.0.2 or id 4.0.0 with NetBox 3.7 support