Validate serverURL on Start #4204

montymxb · 2017-09-23T02:02:23Z

Specifically this adds validation for publicServerUrl, if the value is present. This will attempt to grab the /health endpoint, expecting a 200 code and OK body, if it doesn't receive this it will spit out something like.

...
[1234] parse-server running on http://localhost:1337/parse

WARNING, Unable to connect to publicServerURL 'http://localhost:1337/oops'. Cloud code and push notifications may be unavailable!

This does not add tests yet as I wanted to field the positioning (and wording) of this before I proceed. There may be other systems that would be affected besides cloud code and push.

If this looks ok as is I'll rework it so we can get test coverage on this.

flovilmart · 2017-09-23T02:03:47Z

src/ParseServer.js

@@ -373,6 +373,7 @@ class ParseServer {
    if (process.env.PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS === '1') {
      Parse.CoreManager.setRESTController(ParseServerRESTController(appId, appRouter));
    }
+    this.verifyServerUrl(_ref2.publicServerURL);


Also? You probably need to add a delay, at least a process.nextTick as the app is not mounted/ not listening yet on the port.

Not sure if we can get that event from Express, i’ll Have a look!

That would be ideal to have it execute after an event! As for _ref2, yeah that's gonna get changed before this can be seriously considered, I can't remember where I pulled that from...

Beyond that is this acceptable to put in ParseServer.js or do you think it would be more appropriate elsewhere?

That’s an excellent place to put it, makes total sense. About the event, I may have been mistaken, perhaps we should wait for the ‘mount’ event, not sure if we can have access to the server that listens in the end. Best chance may be a check after 2s no?

We would have to catch when express was listening, but that object is a bit high up. We could do a timing check but I wish we could just get the event. I'll look around a bit more and see if there's anything else, if not we can always do the delay.

I do think we can just listen on the api constructed there, it's later returned, and comes back to be listened under whatever the mount path is, under another express instance. I believe when the first one has listen called on it, it will chain down, and we'll get our listening callback as expected; without being too far ahead.

Looks like it's used in ParseWebSocketServer as such,

... wss.on('listening', () => { logger.info('Parse LiveQuery Server starts running'); });

flovilmart · 2017-09-23T02:09:20Z

Perhaps check the listening event: https://nodejs.org/api/net.html#net_event_listening

Should be available/ fired on the app :)

app.on(‘listening’, () => { verifyServerURL() })

montymxb · 2017-09-23T02:12:05Z

The event sounds much better than sleeping/waiting for a bit, I'll rework this to fire from that, and address that improper var reference.

montymxb · 2017-09-23T02:14:54Z

Doh! I wrote in _ref2 as I was looking at the compiled lib code at that moment, it's a totally garbage value, oops.

flovilmart · 2017-09-25T23:49:37Z

src/ParseServer.js

-    this.verifyServerUrl(_ref2.publicServerURL);
+
+    const AppCache = require('./cache');
+    var serverUrl = AppCache.AppCache.cache[appId]['value']['publicServerURL'];


or just Parse.serverURL ;)

That would be much better

flovilmart · 2017-09-25T23:50:21Z

src/ParseServer.js

+    setTimeout(function() {
+      if(serverUrl) {
+        const request = require('request');
+        request(serverUrl.replace(/\/$/, "") + "/health", function (error, response, body) {


you could use Parse._request probably this way you don't even need those replacements.

And I'll look to see if that'll swap out ok.

acinader

a) love this. thank you
b) ugh, I've become THAT guy™

acinader · 2017-09-25T23:54:23Z

spec/ParseServer.spec.js

@@ -0,0 +1,32 @@
+"use strict";


Please use ' instead of " there are a few places.

acinader · 2017-09-25T23:54:37Z

spec/ParseServer.spec.js

+/* Tests for ParseServer.js */
+var express = require('express');
+
+import ParseServer          from '../src/ParseServer';


remove extra white space

acinader · 2017-09-25T23:54:52Z

spec/ParseServer.spec.js

+import ParseServer          from '../src/ParseServer';
+
+var app = express();
+//app.use(bodyParser.json({ 'type': '*/*' }));


remove commented out code.

acinader · 2017-09-25T23:55:37Z

spec/ParseServer.spec.js

+  it("validate good publicServerUrl", (done) => {
+    ParseServer.verifyServerUrl('http://localhost:12345', function(result) {
+      if(!result) {
+        jfail('Did not pass valid url');


using jfail is fine. we've moved to using done.fail() just for future reference.

acinader · 2017-09-25T23:55:59Z

src/ParseServer.js

@@ -373,6 +373,11 @@ class ParseServer {
    if (process.env.PARSE_SERVER_ENABLE_EXPERIMENTAL_DIRECT_ACCESS === '1') {
      Parse.CoreManager.setRESTController(ParseServerRESTController(appId, appRouter));
    }
+
+    const AppCache = require('./cache');


requires at the top please.

acinader · 2017-09-25T23:57:37Z

src/ParseServer.js

+        request(serverUrl.replace(/\/$/, "") + "/health", function (error, response, body) {
+          if (error || response.statusCode !== 200 || body !== "OK") {
+            // eslint-disable-next-line
+            console.warn("\nWARNING, Unable to connect to publicServerURL '" + serverUrl + "'. Cloud code and push notifications may be unavailable!\n");


use template

`${a-var} blah`

break the code for the comment onto two lines (not the comment).

console.warn(`\nWARNING, Unable to ... \'${serverUrl}\'. Cloud code and ....` + 'blah blah blah'); remove the // eslint-disable

Totally valid nits. I've made the changes locally but I'm still fiddling with something else before I push changes up.

@acinader as for the disable on lint, console statements don't look to be allowed. Ending up with Unexpected console statement no-console. If you take a look you'll see that others have run into the same issue and ignored their console calls as well.

If there's some alternative approach I'm not aware of let me know, but that seems about right.

Following up on that something like /* eslint-disable no-console */ may be more appropriate than just a flat out disable.

ah, yeah. the logger isn't available here. so yes, adding the error your targeting (no-console) is the right call see around line 225 where console is used.

codecov · 2017-09-26T00:39:29Z

Codecov Report

Merging #4204 into master will decrease coverage by 0.01%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #4204      +/-   ##
==========================================
- Coverage   92.22%   92.21%   -0.02%     
==========================================
  Files         116      116              
  Lines        8092     8103      +11     
==========================================
+ Hits         7463     7472       +9     
- Misses        629      631       +2

Impacted Files	Coverage Δ
src/ParseServer.js	`88.3% <81.81%> (-0.45%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6eed955...b8b44f8. Read the comment docs.

montymxb · 2017-09-26T01:08:00Z

Just need to adjust the port I'm listening to for testing. I'm verifying the entire suite locally before I push this up again.

acinader

lgtm

https://hackernoon.com/why-you-shouldnt-use-var-anymore-f109a58b9b70

is a good write-up on why to use let (and ideally const) instead of var.

montymxb · 2017-09-26T16:33:21Z

Thanks @acinader , you'll have to forgive me I'm a bit rusty with my js on node at the moment. Using let/const makes sense over var. I'll go back over those and make the appropriate changes.

@flovilmart I believe things are almost good on my end here (minus the var changes I'll be putting up) . Let me know if there's anything in particular that still requires attention.

As for the failure I'm seeing quite a few of these 'open connection...' errors. At first I thought it had something to do with my using reconfigureServer in the first set of changes. Looks to be that those issues are persisting beyond that, and they seem to be inconsistent, unless I'm missing something that's still changed within these tests. I'm running some more tests locally, and have been seeing intermittent passes/fails on this branch, all regarding the same open connection issue. I haven't confirmed that this happens on the master yet. We may want to hold here until I can confirm there's not some subtle issue being introduced by this.

…ication call into app.on('mount') callback, removed setTimeout from verification.

montymxb · 2017-09-26T19:40:38Z

So I believe I have found the issue, but we'll see for sure once these tests are done.

It appears that running the url verification actually breaks quite a few tests, but only if the connection that is made is still open when a given tests completes, particularly those that use reconfigureServer. This gave the appearance of a random failure, but was most definitely being caused by this additional check after going over things.

To fix this I changed a couple points, one to move the verification call under a conditional that is only executed when not in a testing environment, and two to wrap said call within a mount callback (instead of a fixed timeout). The mount callback is fired directly after this express instance is mounted under another parent instance.

Although the parent is not listening yet, it is assumed it will be before we have the chance to time out (likely to be milliseconds behind). Checking this locally it looks to work just fine. If for some reason it misses this won't cause anything to not run, just would give an eroneous warning. If that ends up being the case in some production instances we could always safely modify this further to be a bit more patient in waiting for a response post-mount.

Those changes looks like so:

// run the following when not testing
if (!process.env.TESTING) {
   ...
   ...
   ...
   
  // verify the server url after a 'mount' event is received
  api.on('mount', function() {
	ParseServer.verifyServerUrl();
  });
}

Since the verification is called separately during testing it is still covered, but now it's out of the way of interfering with the expected behavior of other tests. The only thing not covered being the bit that's inside the conditional above of course, I'm thinking the coverage diff will still be a positive.

Also changed a couple declarations using var to const.

::edit::
The diff is not quite up to snuff, but the tests are now good.

Added basic validation of publicServerURL

7838451

flovilmart suggested changes Sep 23, 2017

View reviewed changes

Fixed up 'verifyServerUrl' and added tests

249f2e5

flovilmart reviewed Sep 25, 2017

View reviewed changes

acinader suggested changes Sep 25, 2017

View reviewed changes

Use Parse.serverURL instead, general cleanup.

9864ec6

montymxb added 2 commits September 25, 2017 18:40

Test server port moved to 13376

149a6f9

Removed reconfigureServer calls with simple changing of Parse.serverURL

930e504

acinader previously approved these changes Sep 26, 2017

View reviewed changes

montymxb added 2 commits September 26, 2017 12:24

changed var to const

299ab4d

Disabled automatic serverURL verification during testing, moved verif…

b8b44f8

…ication call into app.on('mount') callback, removed setTimeout from verification.

montymxb dismissed acinader’s stale review via b8b44f8 September 26, 2017 19:26

montymxb requested a review from flovilmart September 26, 2017 19:49

flovilmart approved these changes Oct 17, 2017

View reviewed changes

flovilmart merged commit 9376b4d into parse-community:master Oct 17, 2017

montymxb deleted the url-health-check branch October 24, 2017 20:13

Uh oh!

Validate serverURL on Start #4204

Validate serverURL on Start #4204

Uh oh!

Conversation

montymxb commented Sep 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flovilmart commented Sep 23, 2017

Uh oh!

montymxb commented Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

montymxb commented Sep 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acinader left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acinader Sep 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

montymxb Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

montymxb commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

acinader left a comment

Choose a reason for hiding this comment

Uh oh!

montymxb commented Sep 26, 2017

Uh oh!

montymxb commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

montymxb commented Sep 23, 2017 •

edited

Loading

acinader Sep 25, 2017 •

edited

Loading

montymxb Sep 26, 2017 •

edited

Loading

codecov bot commented Sep 26, 2017 •

edited

Loading

montymxb commented Sep 26, 2017 •

edited

Loading

montymxb commented Sep 26, 2017 •

edited

Loading