Better error handling, use better practices.

[MikeyBurkman]

This node app is currently doing a lot of weird/bad practices for being an example app, which you almost certainly would never want in any real app:

1. It's potentially initializing the database connection multiple times. (It now would use a single Promise to ensure the database is initialized once and only once.)
2. When connecting to the database, the callback is ONLY called if there's an error
3. If you're missing required Mongo env vars, the app will just silently never initialize the database, which is super confusing. (Yes I know the readme has the required env vars documented, but typos happen, and the code to build the mongoUrl is quite dense.)
4. If a request is made and the db has not yet initialized, the user sees incorrect responses
5. When requesting the home page, it does not wait for the insert call to Mongo to finish before querying for the count, potentially yielding inconsistent results. (Maybe this behavior was intentional? In any case, it was swallowing any errors the DB might have thrown.)
6. Dependencies used only for testing are listed in dependencies rather than devDependencies in package.json
7. Though the readme says Node 4+ is supported, and Node 4 has Object.assign natively, the object-assign polyfill was still being used. (Though it wasn't actually being used in the code?)
8. var was being used all over the place, though it's usage has been largely replaced by const and scope in ES6 code
9. The tests file had different formatting than service.js, and had semicolons only in about half the file

I did NOT fix the tests yet, and they currently fail, so DO NOT MERGE until they have been resolved. Had the tests done anything more substantial than just expect a 200 response from the server, they would have previously failed anyways, as the database would just silently never initialize. (The code branches where db != null were never being reached.)
I didn't fix the tests because I'm not certain which way things should go:

1. Should we mock up MongoClient using proxyquire? (This will look a bit convoluted, as Mongo is not pleasant to correctly mock.)
2. Require the user to spin up an actual instance of Mongo before running the tests, and make these integration tests?
3. Go the more verbose (and more realistic) route, and split out the mongo calls into their own dao module, which can then be mocked up nicely with proxyquire?

[bparees]

If you're missing required Mongo env vars, the app will just silently never initialize the database, which is super confusing.

i haven't looked at all your changes but this behavior is intentional. it should be possible to run the application with no database configured, and still get the welcome page w/ no page view count displayed.

once a DB is added, the pageview count should appear when the page is reloaded.

[MikeyBurkman]

That seems kind of... odd? You'd have to restart the app in order for the env vars to be populated, so why let the app even start up with no indication of why the database isn't working? Even if you wanted the app to be able to start up in an erroneous state, I would think you'd still want to let the dev know what is wrong.

[bparees]

That seems kind of... odd? You'd have to restart the app in order for the env vars to be populated, so why let the app even start up with no indication of why the database isn't working? Even if you wanted the app to be able to start up in an erroneous state, I would think you'd still want to let the dev know what is wrong.

the page does report that no db connection is configured under those conditions. logging something would be nice, sure.

[bparees]

And the reason it's designed this way is this sample app is intended to be deployed both with, and without, a database. For many users trying to get a hello world app running on openshift, they aren't even going to have the DB, so it's important that it "just work" even if you haven't configured a DB.

The DB is an optional feature, not a requirement (which is always why no errors are reported if the DB is not available).

[jimdillon]

Looks to me like @MikeyBurkman has made a very solid contribution. Seems like we should at the very least log that the db was not found, not just put an error on a page served by the app. If we want to show people best practices on using mongodb, this PR does just that.

[bparees]

Seems like we should at the very least log that the db was not found, not just put an error on a page served by the app.

sure, I am just trying to provide some context for why the app behaves as it does.

If we want to show people best practices on using mongodb, this PR does just that.

Not really the intent of the repo, but sure, i'm not against improving it, again I just want the context and goals understood. We have a significant test infrastructure that in part dependent on the behavior of this repo, so changing it needs to be considered carefully.

[MikeyBurkman]

Alright, will change it to make mongo optional for the homepage to display, and will log warnings to the console and fill in the dynamic fields in the homepage appropriately. (I'll see if I can go one further, and if mongo env vars ARE provided, I won't display the page until it's been initialized.)

What about the /pageCount route though? Surely that entire route depends upon the DB being initialized, and we should return a 500 with an appropriate message? Or do other things actually expect to see a -1 returned for the page count?

[bparees]

What about the /pageCount route though? Surely that entire route depends upon the DB being initialized, and we should return a 500 with an appropriate message? Or do other things actually expect to see a -1 returned for the page count?

Yeah i think returning a 500 on that should be ok.

[openshift-bot]

@MikeyBurkman: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[phracek]

Closing this issue. The database were switched from MOngoDB -> Postgresql. Feel free to update it against postgresql.