Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot read property 'getFileData' of undefined #3

Closed
themoonlitknight opened this issue Jul 1, 2016 · 14 comments
Closed

Cannot read property 'getFileData' of undefined #3

themoonlitknight opened this issue Jul 1, 2016 · 14 comments

Comments

@themoonlitknight
Copy link

I'm testing the fs-adapter and it works on creating a file on disk. I got an error when I try to retrieve the file though; this is the stack trace

error: Uncaught internal server error. [TypeError: Cannot read property 'getFileData' of undefined] TypeError: Cannot read property 'getFileData' of undefined at getHandler (/root/parse/node_modules/parse-server/lib/Routers/FilesRouter.js:69:22) at Layer.handle [as handle_request] (/root/parse/node_modules/parse-server/node_modules/express/lib/router/layer.js:95:5) at next (/root/parse/node_modules/parse-server/node_modules/express/lib/router/route.js:131:13) at Route.dispatch (/root/parse/node_modules/parse-server/node_modules/express/lib/router/route.js:112:3) at Layer.handle [as handle_request] (/root/parse/node_modules/parse-server/node_modules/express/lib/router/layer.js:95:5) at /root/parse/node_modules/parse-server/node_modules/express/lib/router/index.js:277:22 at param (/root/parse/node_modules/parse-server/node_modules/express/lib/router/index.js:349:14) at param (/root/parse/node_modules/parse-server/node_modules/express/lib/router/index.js:365:14) at param (/root/parse/node_modules/parse-server/node_modules/express/lib/router/index.js:365:14) at Function.process_params (/root/parse/node_modules/parse-server/node_modules/express/lib/router/index.js:410:3)

I got that when I specify the 'filesSubDirectory' option while creating the adapter. Instead when I leave options blank I receive an {"error": "unauthorized"}

Am I missing something?

@samuelantonioli
Copy link

Hello @themoonlitknight

I had the same problem and after seeing that this issue is still open I just built a new adapter based on this one (also supports optional "other files directory" argument):

https://github.com/samuelantonioli/parse-server-fs-store-adapter

Hopefully it's possible to merge it in the end.


The {"error": "unauthorized"} seems to be from lib/middlewares.js, maybe someone can elaborate

@themoonlitknight
Copy link
Author

Hi @samuelantonioli

I've just tested your module and it seems definitely working. Great job! Thank you for your contribution.

@LucasBadico
Copy link

@themoonlitknight this file adapter works with https domains? I have a big issue with mixed content

@themoonlitknight
Copy link
Author

Unfortunately I haven't tested it yet with https

@samuelantonioli
Copy link

/ping @flovilmart
parse-server-fs-adapter package still gets a lot of downloads (see npm) although it contains bugs.

I've wrote a new one - parse-server-fs-store-adapter - which is essentially the same with all bugs fixed. Please consider to point to it or otherwise let people know that they are downloading a non-working solution.

@flovilmart
Copy link
Contributor

@samuelantonioli, pull requests on the official adapter are welcome.

@samuelantonioli
Copy link

I would like to remove the mkdir functionality because it looks way too hacky and uses *Sync functions (in this function and here). I would prefer that people just create the directory that they want to use themselves.

Would you accept such a pull request? (which essentially drops the mkdir functionality)

@flovilmart
Copy link
Contributor

I’m not sure how removing such functionality is fixing the issue. I’d rather fix it, add proper tests etc...

You were mentioning that this project had ‘many’ issues, which are the others?

@samuelantonioli
Copy link

I think it's better if the module only checks the existence of the directory in the constructor (and not on each request - see _getLocalFilePath). It wouldn't be too bad to use the Sync-functions then because it will only be executed on start-up.

I still think that it's easy to let some subtle bugs in the mkdir functionality slip and I think it's ok to avoid error-prone code and just give people clear instructions on the usage. But I wouldn't say no to mkdir functionality if it only executes it in the constructor and gets some tests.

I didn't say many (the code isn't that long) - I'm not trying to be negative.
There are subtle bugs

      fs.unlink(filepath, (unlinkErr) => {
      if(err !== null) {    // <- should be unlinkErr
          return reject(unlinkErr);
        }

and the indentation is not consistent. There are also things that I don't like e.g. it calls _getApplicationDir on each request which builds the same path over and over (instead of calling it once in the constructor and reuse it).

@flovilmart
Copy link
Contributor

You know what’s great, it’s opensource, and you can open a / many pull requests.

@samuelantonioli
Copy link

You know what’s great, it’s opensource, and you can open a / many pull requests.

No problem. It just seemed that this is a stalled project (because of no reactions in the issues and no code updates). And honestly, this ("You know what’s great") sounds snarky.

Thousands of people download the package every week and get disappointed. This is not a good first impression for the whole parse-server project. I just try to help here.


I can add mkdir functionality in the code of parse-server-fs-store-adapter and make a PR. I now know that the project isn't stalled.

@flovilmart
Copy link
Contributor

Look, we have 30+ repositories to maintain, and local file system storage for files isn’t a recommended method as it causes issues as soon as you use more than 2 instances. The issue was opened a while ago, and yes, we didn’t had a big look on it as it’s minor and all intégration tests pass. If you’re eager to help out, again feel free to open a PR.

@flovilmart
Copy link
Contributor

and I’m not in favor removing the mkdir functionality as it’s a breaking change.

@samuelantonioli
Copy link

I understand you. As I've said, will do (with mkdir).

I think you as one of the core developers of parse-server are doing a great job and I appreciate what you and the other team members built. I'll see if I can help with other issues in the Parse project family.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants