EVENT_UPLOAD_COMPLETE wrong payload when using GCS

[tkrugg]

It seems when using GCS, the payload of the EVENT_UPLOAD_COMPLETE event is not as stated in the README.md

{
   file: {
        id: '7b26bf4d22cf7198d3b3706bf0379794',
        upload_length: '41767441',
        upload_metadata: 'filename NDFfbWIubXA0'
    }
}

instead, file is actually an instance of the File class as you can tell by the code: https://github.com/tus/tus-node-server/blob/master/lib/stores/GCSDataStore.js#L160

[Acconut]

Thanks for brining this up. It seems as if that's also the case for other events, such as https://github.com/tus/tus-node-server/blob/master/lib/stores/GCSDataStore.js#L111

Would you mind sending a pull request to fix this issue?

[tkrugg]

Sorry for the late reply. Sure, will do.

[Acconut]

No worries, looking forward to your contribution.

[tkrugg]

Hi @Acconut

It seems as if that's also the case for other events, such as https://github.com/tus/tus-node-server/blob/master/lib/stores/GCSDataStore.js#L111

Looking at it again it seems the payload of EVENT_FILE_CREATED is consistently an instance of models/File for DataStore, GCSDataStore and S3Store.

The appear to be inconsistant for EVENT_UPLOAD_COMPLETE however

• S3Store conforms to the doc
• FileStore and GCSDataStore use a different payload.file

I'm not sure on whether we should clarify the docs or fix this inconsistency

[Acconut]

Thank you for the detailed update. Frankly, I am not sure what the best next step. I would like to keep the interface and documentation across all file storages consistent. However, some people might rely on the GCSDataStore- and FileStore-specific values in the event, so changing its type might break their application and I would like to avoid a breaking change, if possible.

Maybe it is possible to extend the objects from FileStore and GCSDataStore to also have the id, upload_length and upload_metadata properties. What do you think?

[tkrugg]

I think that could work although that isn't really more cautious since we don't know what people do with the file instance after EVENT_UPLOAD_COMPLETE has returned it 😄 I would personally introduce a breaking change and release it in a new major, have a payload where file is consistently an instance of models/file, which will break FileStore and GCSDataStore Then we can add payload.file.remoteInstance ? migrating code from file to file.remoteInstance shouldn't be too painful for the users, but it's your call.

[Acconut]

I agree, a major release is needed in the end. However, I would like to make it as easy as possible for users to be aware of this change, meaning that I would do following:

• Release a new patch release where the payload object is fitted with the additional properties as defined in the documentation. This should allow it to be backwards compatible but also keeps the documentation correct. Furthermore, a deprecation warning should be printed if the users adds an event listener for EVENT_UPLOAD_COMPLETE to FileStore/GCSStore, saying that this behavior will be changed in the next major release.
• Release a new major release where the deprecation is removed and the event payload is a defined by the documentation. Furthermore, the payload should provide details about the underlying storage object (as you meant by remoteInstance).

How does this sound to you?

[Murderlon]

Closed in #344