Skip to content

Does parse-server call next() or not? #4085

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

Closed
woodardj opened this issue Aug 7, 2017 · 11 comments
Closed

Does parse-server call next() or not? #4085

woodardj opened this issue Aug 7, 2017 · 11 comments

Comments

@woodardj
Copy link

woodardj commented Aug 7, 2017

At the very beginning of its open source life, ParseServer was not calling next() to hand off execution to later middlewares — then (if my notes are correct), a change with version 2.2.16 ParseServer began calling next() after execution. We implemented a middleware directly after as a stopgap measure until we could properly deal with the fallthrough. But now it appears our middleware is not being called on requests that are Parse queries. Here's our setup:

    var api = new ParseServer({/*...*/});

    var mountPath = process.env.PARSE_MOUNT || "/parse";
    app.use(mountPath, api);

    app.use(function (req, res, next) {
        // !! NEVER EXECUTES WHEN PATH IS /parse/classes/<className> OR /parse/functions/<functionName>
        var path = req.path.split('/');
        var req_id = req.headers['x-request-id'];
        console.log(req_id + " checking parse-next() middleware through " + req.path ); 
        // ...
    });

Currently using ParseServer 2.4.2. We're hoping to set up some server-wide instrumentation, which would require executing middlewares after parse-server even on routes it handled.

Thanks!

@nbering
Copy link

nbering commented Aug 7, 2017

Parse Server uses it's own little promise-based router which is built on top of Express' own routing engine. This is the function that eventually resolves all internal Parse Server routes:

function makeExpressHandler(appId, promiseHandler) {
return function(req, res, next) {
try {
const url = maskSensitiveUrl(req);
const body = Object.assign({}, req.body);
const stringifiedBody = JSON.stringify(body, null, 2);
log.verbose(`REQUEST for [${req.method}] ${url}: ${stringifiedBody}`, {
method: req.method,
url: url,
headers: req.headers,
body: body
});
promiseHandler(req).then((result) => {
if (!result.response && !result.location && !result.text) {
log.error('the handler did not include a "response" or a "location" field');
throw 'control should not get here';
}
const stringifiedResponse = JSON.stringify(result, null, 2);
log.verbose(
`RESPONSE from [${req.method}] ${url}: ${stringifiedResponse}`,
{result: result}
);
var status = result.status || 200;
res.status(status);
if (result.text) {
res.send(result.text);
return;
}
if (result.location) {
res.set('Location', result.location);
// Override the default expressjs response
// as it double encodes %encoded chars in URL
if (!result.response) {
res.send('Found. Redirecting to ' + result.location);
return;
}
}
if (result.headers) {
Object.keys(result.headers).forEach((header) => {
res.set(header, result.headers[header]);
})
}
res.json(result.response);
}, (e) => {
log.error(`Error generating response. ${inspect(e)}`, {error: e});
next(e);
});
} catch (e) {
log.error(`Error handling request: ${inspect(e)}`, {error: e});
next(e);
}
}
}

As far as I understand it (which is pretty well since I borrowed this pattern for an app I'm working on), every internal route resolves to this handler function. It always creates a response, whether it be text, redirect, or JSON.

I'm not an expert on Parse Server, but as far as I can tell the answer to your question is "No, Parse Server does not call next()." At least not on anything that passes through the Promise Router.

I could be wrong, though. It's a big code-base and I haven't been through the whole thing.

@nbering
Copy link

nbering commented Aug 7, 2017

One small correction... I guess. If it's an error, next is called with the error. But if there's a successful response, any middleware you add after Parse will be skipped.

@steven-supersolid
Copy link
Contributor

Looks like calling next() was introduced in 2.2.17 with #2338 and reverted in 2.2.10 with #2634.

Please see comments in the revert for more info but issues #2362 and #2580 are cited.

I believe there is a workaround mentioned in another issue by wrapping the parse mount in your own middleware that can then implement logic before and after the parse logic.

@woodardj
Copy link
Author

Ah yes, I weighed in on the surprise behavior change on #2362 (#2362 (comment)) myself — though the suggested fix (#2362 (comment)) to halt execution and maintain 2.2.16 behavior worked great.

Since it's billed as "middleware", it really ought to pass through, but I guess I missed the window to join that debate.

@flovilmart
Copy link
Contributor

Closing, I believe we should not call next as everything is handled internally. Happy to reconsider at a later date if enough arguments can be made !

@woodardj
Copy link
Author

So when I see poor performance from endpoints that Parse Server 'handles', I should dig in on those… how?

@flovilmart
Copy link
Contributor

flovilmart commented Sep 10, 2017

we use connect-datadog for performance monitoring, I also recently used both NewRelic and AppDynamics for performance monitoring, I'm not sure what prevents you from doing the same.

parse-server is not a middleware, it's a full blown express app.

See: https://github.com/AppPress/node-connect-datadog/blob/master/lib/index.js#L11

Overriding req.end is usually the way to go to monitor your request performance.

@woodardj
Copy link
Author

woodardj commented Nov 1, 2017

Sorry to pick up a convo on a closed thread — when I've installed those tools (used NewRelic for a bit, using OpBeat now), the level of granularity I get is always no better than:

screen shot 2017-11-01 at 11 33 12 am

i.e. things within the parse-server application are totally opaque, so I can't zero in on whether it's a slow mongo query, or slow assembly of mongo results, etc.

Do you have a different experience with these tools and parse-server? If so, I guess I should figure out what's wrong with my configuration :-\

@flovilmart
Copy link
Contributor

It’s usually because your injection of newrelic or other is not the 1st require.

@woodardj
Copy link
Author

woodardj commented Nov 1, 2017

Definitely first thing.
Procfile:

web: bin/webstart
worker: bin/workstart

bin/webstart:

if [ -z "$SERVER_URL" ]; then
    export SERVER_URL=https://$HEROKU_APP_NAME.herokuapp.com/parse
fi

if [ -z "$STRIPE_MODE_IS_TEST_OR_LIVE" ]; then
    export STRIPE_MODE_IS_TEST_OR_LIVE=TEST
fi

node --expose_gc --optimize_for_size --max_old_space_size=960 --gc_interval=100 index.js

index.js:
(newrelic where we tried that a while back, then commented out)

var opbeat = require('opbeat').start({'active':(process.env.ENVIRONMENT_STAGE !== 'development')});
// if (typeof(process.env.HEROKU_APP_NAME) === "undefined") { // Disable newrelic for review apps
// require('newrelic'); // First so we get instrumentation all over.
// }

🤷‍♂️
So, I guess we're doing something else wrong when invoking this. Thanks for the insight, since this is clearly outside scope of parse-server, but it is good to know these tools work better than what I'm seeing for node. Any other thoughts welcome, though.

@flovilmart
Copy link
Contributor

I have no particular insight on opbeats, but we manage to successfully use either appDynamics, newRelic or even google cloud trace without any issue, as long as we loaded this module first.

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

4 participants