Backend Troubles

[Fripplebubby]

The current backend has a number of problems at the moment - some are easy to fix but others are more conceptual.

Little Problems

1. Syntax errors committed to master:

Switched to branch 'master'
Your branch is up-to-date with 'origin/master'.
$ node index.js
/home/voynich/PeopleMakingADifference/Backend/index.js:135
		if (err) throw err;)}
SyntaxError: Unexpected token )
...

Not a big problem, just some typos and mismatched braces etc. But in the future we have to make sure code doesn't make it to the master branch in this state. Test first! Team members working on other parts of the app shouldn't have to debug just to get into a working state.

2. Get_message endpoint doesn't work
   There are two problems with the get_message endpoint. First, it's unreachable. We'll cover that later. More directly, we get:

ReferenceError: db is not defined
    at /home/voynich/PeopleMakingADifference/Backend/index.js:115:3
   ...

This is because get_message attempts to get data from the database (the "db" object) without establishing a connection to the database first. To fix this, wrap the code in:

mongodb.MongoClient.connect(uri, function(err, db){
  // database transaction code goes here!
});

3. Style and whitespace are a mess
   Not a big deal really, but a lot of the code looks like it's just been copy-pasted, which I assume it has (which is fine! just fix the spacing).

4. Unneccessary code duplication
   As a quick fix, the CORS headers have been set for every endpoint. Some issues:

res.header('Access-Control-Allow-Origin', req.headers.origin);
res.header("Access-Control-Allow-Origin", "*");

Here, we're setting the Access-Control-Allow-Origin header to match the request origin, and then overwriting it on the next line with the wildcard selector. Both options are functionally equivalent. I think using the wildcard is the best practice because it communicates intent.

res.header('Access-Control-Allow-Methods', 'GET,PUT,POST,DELETE');

Here, the endpoint is saying that it can handle GET, PUT, POST, & DELETE requests, but this is untrue. It actually only supports GET & POST. This is the same across all the endpoints.

Last, the code to set these headers is copied and pasted into the function handler for every endpoint. It is much simpler to tell Express that we want these headers on all our endpoints by default:

app.use(function(req, res, next) {
  res.header("Access-Control-Allow-Origin", "*");
  res.header("Access-Control-Allow-Headers", "Origin, X-Requested-With, Content-Type, Accept");
  next();
});

This way, we don't have to duplicate that code. Express will call this function for every request before it gets to the endpoint callbacks - next() is where it goes to the actual endpoint code.

Big picture problems

1. The GET endpoint for volunteers by ID is too greedy

The endpoint for getting volunteer data is defined as:

app.get("/:uid", function(req, res){
// retrieval code...
}

What this means is that for any GET request to any url other than '/', whatever is after the '/' is saved as the "uid" variable.
So a GET to '/3' sets the uid -> 3, etc.
This also means that a GET to '/get_message' sets the uid -> 'get_message'!
Doing a GET to '/get_message', where we expect the server message to be returned, results in:

Error: UID Not Found!

Because it attempts to find a volunteer with an id of parseInt(req.params.uid) = NaN.

To fix this, we need to redefine the endpoint as something like

app.get("/volunteer/:uid", function(req, res){
// retrieval code...
}

And update the code on the mobile and desktop to use that endpoint instead.
It's also important not to push this update without the mobile and desktop teams being ready for it, because this is a breaking change - both versions of our app will need to be updated to work properly!

2. Populating the database
   To populate the database with our dummy data, the user first needs to run

$ node app.js

This is not documented anywhere. It also doesn't add a "message" - we should have a default message to prevent errors elsewhere.
3. Error handling
On the volunteer endpoint, if provide a valid input:

GET '/3'
Content-Type: application/json
[{"_id":"59ffaabde624c045fb694782","id":2,"name":"Darth Vader","assignment":"Use the Force (For evil)","location":"The Death Star"}]

We get a JSON document in return, just as expected. But if we provide an invalid input:

GET '/7'
Content-Type: text/html
Error: UID Not Found!

We get an HTML response. This makes things more complicated for the mobile & desktop apps. The best practice would be to return a JSON response, even if we have an error:

GET '/7'
Content-Type: application/json
{"error": true, "message": "Error: UID Not Found!"}

That way, we can always expect (and parse) a JSON response - if we don't get JSON, that means we have a server error, but if we get a JSON response with an error in it, that means we have an "expected error", like an invalid ID.