enonic / xp

Enonic XP
https://enonic.com
GNU General Public License v3.0
202 stars 34 forks source link

contentLib.removeAttachment won't remove data property #7910

Open alansemenov opened 4 years ago

alansemenov commented 4 years ago

Triggered by a case on Discuss: https://discuss.enonic.com/t/contentlib-removeattachment-is-not-removing-attachments/2134

After an attachment is uploaded from Content Studio, content looks like this:

{
    "_id": "f899a2bb-809c-4eda-a778-a531b54bccad",
    ...
    "data": {
        "myAttachment": "_DSC0038.JPG"
    },
    "attachments": {
        "_DSC0038.JPG": {
            "name": "_DSC0038.JPG",
            "size": 6626427,
            "mimeType": "image/jpeg"
        }
    }
}

Note data.myAttachment property with attachment name in addition to binary attachment.

If one then executes contentLib.removeAttachment to remove the attachment:

    var contentLib = require('/lib/xp/content');
    contentLib.removeAttachment({
        key: "f899a2bb-809c-4eda-a778-a531b54bccad",
        name: result.data.myAttachment
    });

then the binary will be removed but not data.myAttachment:

{
    "_id": "f899a2bb-809c-4eda-a778-a531b54bccad",
    ...
    "data": {
        "myAttachment": "_DSC0038.JPG"
    },
    "attachments": {}
}

As a result, in Content Studio it will still look as if the attachment is present on the content, but it will produce an error when clicking on the attachment.

This is inconsistent behaviour. We should either fix API to also create and remove a data property, or fix Content Studio to not create/remove it.

rymsha commented 4 years ago

It looks like Content Studio adds property to data itself, so content has no knowledge about this data property. Content lib should not delete properties it doesn't create. IMO - it is not a bug.

sigdestad commented 4 years ago

Hmm.. But th only way to "add an attachment" is using the attachment input type. This input type does indeed know about the properties. However, the add/remove attachment is a supporting function for managing all attachments for a given item. So - you might have a dosen attanchment fields, but there is only one "list" of actual attachments vs potentially many properties referencing them.

sigdestad commented 4 years ago

Lets discuss this, book a meeting @alansemenov

alansemenov commented 4 years ago

The behaviour is correct. If attachment is uploaded via API, it should be deleted via API as well. If attachment is uploaded via Content Studio and removed via API, the data property will remain.

The fix here is to check on content save if attachment that the data property refers to still exists. If attachment doesn't exist, data property must be removed.

rymsha commented 4 years ago

According to #8225 (unrelated issue) "Form Builder" uses _attachments field. So from content_lib point of view _attachments or attachments filed is something lib users "made up", and it is up to lib user to remove the field when she removes an attachment.