FileAdapter errors ignored

[simonbengtsson]

If a FileAdapter returns an error it is later ignored in the file router:

// FileRouter.js
}).catch((err) => {
  // err is ignored
  next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, "Could not store file"));
});

The S3Adapter for example returns an error with the following message when the keys are wrong: The request signature we calculated does not match the signature you provided. Check your key and signing method. Would it somehow be possible to propagate this error to the developer?

[simonbengtsson]

I thought of making a PR, but I'm not entirely sure what would be best here. Simply replacing the "Could not store file" message with the one from the adapter like this?

// FileRouter.js
}).catch((err) => {
  next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, err));
});

[chriscborg]

Hi @simonbengtsson - This line worked for me if it helps:

// FileRouter.js
}).catch((err) => {
  next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file: ' + err));
});

So I guess your code should work as well.

[simonbengtsson]

The question I guess is how much the adapters should be able to choose what error messages get sent to the developer. Probably it makes sense to let the adapters fully decide. It will require some changes to the adapters though to make them send better formatted errors.

[flovilmart]

There will be a need in the files adapter to return a 'standard' response as for now, when you save through the GridStore adapter or the S3Adpater, the responses differs. The FilesController is ignoring that response and returning a pre-cooked one.

[simonbengtsson]

Should the goal be that every subclass of FileAdapter should return a Parse.Error that can be passed directly to the user?

[flovilmart]

that would be nice to have something standard yes, doesn't have to be a Parse.Error as the *Controller is responsible for formatting

[parthtiwari65]

Where can I find the file, " FileRouter.js " to make these changes?

I don't see this file in my Parse server copy.

[hawkit]

@flovilmart I have changed FileRouter.js as following : next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file: ' + err)); at EC2 instance. but why i still get same error as

[Error]: Could not store file. (Code: 130, Version: 1.12.0)

Do I need to restart EBS to make it work?

[jdzorz]

@parthtiwari65 it is under node_modules/parse-server/lib/controllers/FilesController.js

@hawkit yes restart your EBS

[hawkit]

@parthtiwari65 it is here: /var/app/current/node_modules/parse-server/lib/Routers/FilesRouter.js

[hawkit]

@jdzorz thanks a lot. It works!

[wavo89]

I'm using Heroku- how do I make sure that this change is active on the server? When I changed it and went to git add/commit/push, it said that there weren't any changes to save.

[abbasdawood]

@jdzorz I can't seem to get this fixed even with parse-server (latest), on modifying the default error message this is what I see on console

ParseError { code: 130, message: 'Could not store file.InvalidParameterType: Expected params.ContentType to be a string' }

I understand the Parse.File constructor takes in the MIME type as the 3rd parameter, so I set this manually - which didn't help either. I also think there were some modifications in 2.1.3 to auto detect the MIME type of the uploaded file, so I don't know why MIME has to be provided anyway.

Don't think anything is wrong with the S3 configuration / policy, since images are saving properly. This particular issue is only for .csv files.

Any help would really be appreciated :)

[abbasdawood]

Guys, Solved the issue - seems I had initialized the S3 Adapter like so:

var S3Adapter = require('parse-server').S3Adapter;

And I had changed to

var S3Adapter = require('parse-server-s3-adapter');

Now I've reverted to the older one and it works just fine, no clue why though

[hramos]

@simonbengtsson is this still an issue?

[simonbengtsson]

I'm not sure, but I think so based on the code in /src/Routers/FilesRouter.js#L91

filesController.createFile(config, filename, req.body, contentType).then((result) => {
  res.status(201);
  res.set('Location', result.url);
  res.json(result);
}).catch((err) => {
  // err is still ignored here
  next(new Parse.Error(Parse.Error.FILE_SAVE_ERROR, 'Could not store file.'));
});

It's not really an issue, but a helpful error message get ignored and it would be nice it was presented to the user.

[lewellyn]

@hramos This is indeed still an issue. I just had to apply the suggestion by @chriscborg to expose that I was running into #1002 because the error I was getting was completely useless.

I'd create a PR except that I'm not sure it's the "right" answer, even as an interim measure, based on the discussion above with @flovilmart. On the other hand, my gut says that any breakage and confusion caused by exposing the error is more than outweighed by the benefit of no longer blindly eating a fatal error.

My biggest concern is that this could potentially be used to get an adapter to dump sensitive information which it would otherwise never show. I've never audited any of the adapters to know how real a risk exists here.

[lewellyn]

Oh, to elaborate... I went with @chriscborg's answer instead of @simonbengtsson's because:

1. I like the fact that it explicitly says the file was not saved.
2. The output is more friendly.

For example:

{"code":130,"error":{"errno":-13,"code":"EACCES","syscall":"open","path":"files/some/dir/path/4a1b30ba492d98c7842d7559742e46e5_Test_file.bin"}}

versus

{"code":130,"error":"Could not store file: Error: EACCES: permission denied, open 'files/some/dir/path/f4f01543df5d711ab1cc9c058218227c_Test_file.bin'"}

I sincerely doubt anyone will be writing an error parser for the result of a failed upload. It's more likely that the result will be logged/displayed/some-other-thing-a-human-should-deal-with-directly. Besides, different adapters are likely to have different types of results. Showing them as-is is probably not the best approach here.

(Yes, I generated a different error than the one I originally was troubleshooting. But it explains the difference between the two approaches better.)

[hubertott]

@abbasdawood if this was indeed your problem, perhaps we should be updating the example on this page.

https://github.com/ParsePlatform/parse-server/wiki/Configuring-File-Adapters#passing-as-options

[acinader]

I think I have solved this with: #3424

BUUUUT, read on, cause i think that there is more to do here....

So this one has been bugging me for awhile :).

Sometimes, an error will result in a very unhelpful: [object Object] output on console.error.

1. Stuff should not be output on console.error. Users of parse-server should expect that errors will be output to the logger, not the console.

2. the error message is totally useless!

The output is coming from: the 'backstop' next() handler that is called at the end of an express request if 'something' isn't done to prevent it.

this calls this

In my case, the error was due to a misconfigured s3 adapter. And I fixed the lack of this particular error with this change: #3424

But I KNOW that there are other places where the dreaded [object Object] output to console.error occurs and it would be good if we could put in a good backstop.

SOOOOO, I see that middleware.js gets called, generates a response and then calls the final backstop in express.

With a nice TODO: // TODO: Add logging as those errors won't make it to the PromiseRouter

it would seem to me that three changes are needed to make a general solution that will allow us to remove that todo:

1. Extend Parse.Error (Parse.ServerError?) to add the optional param 'Source' with the source error.

2. log the source error in middlewares.handleParseErrors

3. find all places where we new Parse.Error change to Paerse.ServerError and add a source error if/when appropriate...

4. remove the next() from the end of the middlewares.handleParseErrors.

I'm game to do it, but want to make sure I am on the right track since I'm no express expert...yet :).

PS, it would also seem reasonable to redefine console.log, console.warn, console.error to logger.info....etc.?

[flovilmart]

@acinader, on the paper, renaming console.* makes sense, however, that could lead to unexpected behavior from cloud code for ex.

[acinader]

@flovilmart ok, agreed, overriding console will likely cause all kinds of havoc. how about extending Parse.Error with Parse.ServerError? Sound good to you?

[flovilmart]

Or plain Error no? There was some complaints that Parse.Error is not a subclass of Error.

[acinader]

k. buy that too. i'll take a crack at it.

so i think we can hold on #3424 for now. if this works, then we can close it.

[flovilmart]

Excellent!