dimer47 / node-ovh-objectstorage

Creative Commons Attribution Share Alike 4.0 International
13 stars 11 forks source link

With "Get object content" binary content are corrupted. Set "encoding: null" in request options to fix it. #10

Closed Relik77 closed 4 years ago

Relik77 commented 4 years ago

file src/Objects.js lines 126 to 142 :

request({
    method: 'GET',
    uri: this.context.endpoint.url + "/" + file,
    headers: {
        "X-Auth-Token": this.context.token,
        "Accept": "application/json"
    },
    encoding: null     // ** Add this line **
}, (err, res, body) => {
    [...]
})
dimer47 commented 4 years ago

Hello Relik77,

After testing I doesn't have the file content corrupted when I using the OVHStorage(config).objects().get(file path) method.

Can you tell me more ?

Thank you in advance for your reply

Relik77 commented 4 years ago

Hi,

For my project, I needed to recover the content of a file (image at this time, but can be of any type) in order to return it by an API (without saving it locally). So I use the "Get object content" method. But after multiple tests, the received file is returned in bad encoding.

It turns out that the https://www.npmjs.com/package/request module works like this:

encoding - encoding to be used on setEncoding of response data. If null, the body is returned as a Buffer. Anything else (including the default value of undefined) will be passed as the encoding parameter to toString() (meaning this is effectively utf8 by default). (Note: if you expect binary data, you should set encoding: null.)

So I need the file in binary, but it is not possible to convert data from utf8 to binary lossless.

Hence make it added "encoding: null" in request options

Mattia-Longhin commented 4 years ago

Hi @dimer47, First of all, thank you for your work.

Concerning this issue, @Relik77 is right. I had exactly the same problem and his solution seems to be the right one.

@Relik77, you could maybe submit à PR?

Relik77 commented 4 years ago

@Mattia-Longhin Good suggestion, I just submitted a PR.

dimer47 commented 4 years ago

Hi @Relik77, Hi @Mattia-Longhin

Thank you for feedback. It's very cool have to contributers in this package.

@Relik77 I have see your PR concerning get binary content of object files, I want to propose you an alternative to define encoding in params of get() method, like :

    /**
     * Get file
     *
     * @param {String} path Path of file with container
     * @param {String|null|undefined} encoding If null, the body is returned as a Buffer by default it is utf8.
     *
     * @async
     * @return {Promise<{content: *, headers: Objects}>}
     */
    get(path, encoding = undefined) {
        return new Promise(async (resolve, reject) => {
            try {
                // check
                if (_.isUndefined(path)) // noinspection ExceptionCaughtLocallyJS
                    throw new Error("Path parameter is expected.");
                if (!_.isString(path)) // noinspection ExceptionCaughtLocallyJS
                    throw new Error("Path parameter is not a string.");
                if (!_.includes(path, '/')) // noinspection ExceptionCaughtLocallyJS
                    throw new Error("Path parameter isn't valid : container/filename.ext.");

                // check if file exist
                if (!(await this.context.objects().exist(path))) // noinspection ExceptionCaughtLocallyJS
                    throw new Error("File path does not seem to exist.");

                let file = (() => {
                    let p = path.split('/');
                    if (p[0] === "")
                        delete p[0];

                    p = _.filter(p, (r) => {
                        return !_.isUndefined(r);
                    });

                    return p.join("/");
                })();

                request({
                    method: 'GET',
                    uri: this.context.endpoint.url + "/" + file,
                    headers: {
                        "X-Auth-Token": this.context.token,
                        "Accept": "application/json"
                    }, 
                    encoding: encoding
                }, (err, res, body) => {
                    err = err || request.checkIfResponseIsError(res);
                    if (err) // noinspection ExceptionCaughtLocallyJS
                        throw new Error(err);

                    return resolve({
                        'content': body,
                        'headers': res.headers
                    });
                })
            } catch (e) {
                return reject(e);
            }
        });
    }

For me, this alternative it more modular, more open. I hope you will agree ?

Relik77 commented 4 years ago

I agree with this solution. I updated corresponding PR.