JSON API in C++ with Node.native, RapidJSON, and MySQL

Question

I was hoping I might get some feedback and ideas for improvement on this, particularly the WebRouter class.

#include <native/native.h>
#include <rapidjson/writer.h>
#include <rapidjson/stringbuffer.h>
#include <cppconn/driver.h>
#include <cppconn/connection.h>
#include <cppconn/statement.h>
#include <cppconn/prepared_statement.h>
#include <cppconn/resultset.h>
#include <cppconn/exception.h>
#include <cppconn/warning.h>
#include <market_config.hpp>

using namespace native::http;
namespace json = rapidjson;

const std::string database(DATABASE);
const std::string url(DBHOST);
const std::string user(USER);
const std::string password(PASSWORD);

typedef std::shared_ptr<sql::Connection> SharedConnectionPtr;

class ConnectionPool {
    public:
       ConnectionPool(uint32_t max_connections) {
           for (uint32_t i = 0; i < max_connections; i++) {
               SharedConnectionPtr con = createConnection();
               free_connections.push_back(con);
           }
       }

       SharedConnectionPtr getConnection() {
           SharedConnectionPtr con = *(free_connections.end() - 1);
           free_connections.pop_back();
           return con;
       }

       void releaseConnection(SharedConnectionPtr con) {
           free_connections.push_back(con);
       }

    private:
        std::vector<SharedConnectionPtr> free_connections;

        SharedConnectionPtr createConnection() {
            try { 
                sql::Driver *driver;
                driver = get_driver_instance();
                auto con = std::shared_ptr<sql::Connection>(driver->connect(url, user, password));
                return con;
            } catch (sql::SQLException) { 
                std::cout << "Failed to connecto to database. Exiting..." << std::endl;
                exit(1);
            }
        }
};

class WebRouter { 
    public:
        WebRouter() {
            pool = new ConnectionPool(10);
        }

        void serve(request& req, response& res) { 
            if ( req.url().path() == "/" ) { 
                getIndex(req, res);
            } else if ( req.url().path() == "/history/AA" )  { 
                getHistory(req, res);
            } else { 
                res.set_status(404);
                res.set_header("Content-Type", "text/plain");
                res.end("Not found.");
            }
        }
    private: 
        ConnectionPool *pool;

        void getIndex(request& req, response& res) { 
            res.set_status(200);
            res.set_header("Content-Type", "text/plain");
            res.end("This is the index!");
        }

        void getHistory(request& req, response& res) { 
            SharedConnectionPtr con;
            sql::Statement *stmt;
            sql::ResultSet *result;

            json::StringBuffer buffer;
            json::Writer<json::StringBuffer> writer(buffer);

            con = pool->getConnection();
            con->setSchema(DATABASE);
            stmt = con->createStatement();

            result = stmt->executeQuery("select * from daily_history where symbol_id = 'AA';");

            writer.StartArray();
                while (result->next()) {
                    writer.StartArray();
                        writer.Int(result->getInt(2));       // timestamp
                        writer.Double(result->getDouble(3)); // open
                        writer.Double(result->getDouble(4)); // high
                        writer.Double(result->getDouble(5)); // low
                        writer.Double(result->getDouble(6)); // close
                        writer.Uint(result->getUInt(7));     // volume
                    writer.EndArray();
                }
            writer.EndArray();

            delete result;
            delete stmt;
            pool->releaseConnection(con);

            res.set_status(200);
            res.set_header("Content-Type", "application/json");
            res.end(buffer.GetString());
        }
};

int main() {
    http server;
    WebRouter *router = new WebRouter(); 
    if(!server.listen("0.0.0.0", 8080, [&](request& req, response& res) {
        router->serve(req, res);
    })) return 1; 

    return native::run();
}

Answer 1

I know this is a small app, but I would move the direct SQL calls into it's own class:

From this:

sql::ResultSet *result;
result = stmt->executeQuery("select * from daily_history where symbol_id = 'AA';");

To this:

DailyHistoryFactory *daily_history_factory;
daily_history_factory = new DailyHistoryFactory();

sql::ResultSet *result;
result = daily_history_factory->all_daily_histories();

The factory could be responsible for connecting as well, possibly through subclassing of the Storage layer.

I see a code smell as well: knowledge built into comments.

writer.Int(result->getInt(2));       // timestamp
writer.Double(result->getDouble(3)); // open
writer.Double(result->getDouble(4)); // high
writer.Double(result->getDouble(5)); // low
writer.Double(result->getDouble(6)); // close
writer.Uint(result->getUInt(7));     // volume

I would wrap the building of this array into a new class as well. You can have a build method inside that class:

class ResponseBuilder {
  public:
    json::StringBuffer build(sql::ResultSet result) {
      builder.add_timestamp(result);  // Builder is an instance variable you can instantiate
      builder.add_open(result);       // internally that does the string building.
      builder.add_high(result);
      builder.add_low(result);
      builder.add_close(result);
      builder.add_volume(result);
      return builder.GetString();
    }
};

This way, you are capturing the purpose of those calls into the code itself. If you have to comment, the code might change and comments might not be updated. Better to build it into the code directly. Now it is easy to read.

You can also store "result" inside the ResponseBuilder when you initialize it, so each instance method can access it without needing it passed in like I have here.

Answer 2

First Impression

You are not using RAII (basically use your constructor/destructor to resource management).

This leads to Rule of three (rule of five C++11). Your WebRouter class does not obey the rule of three. As a result it is very dangerous (thus must be fixed). Basically when your class contains an owned pointer you must implement (or disable) the "copy constructor", "assignment operator" and destructor.

Details

Don't do this:

using namespace native::http;

See here. Same rules apply to other namespace not just std.

This is good.

namespace json = rapidjson;

You should have used the same technique for native::http.

Careful here.

       SharedConnectionPtr getConnection() {
           SharedConnectionPtr con = *(free_connections.end() - 1);
           free_connections.pop_back();
           return con;
       }

If your connection pool is empty then the above is undefined behavior.
You should be checking that there is an available member in the pool and take remedial action if there are non available.

This is total fine:

        std::vector<SharedConnectionPtr> free_connections;

But for this situation I would prefer to use boost::ptr_vector<sql::Connection>. I would put ownership of the connection with the class and only return references externally.

Your WebRouter is not safe to use:

class WebRouter { 
        ConnectionPool *pool;
    public:
        WebRouter() {
            pool = new ConnectionPool(10);
        }

If you do this:

{
      WebRouter  a;
      WebRouter  b(a);
}
// Your universe explodes in nasal dragons here.

This is because you do not define a copy constructor. The default (compiler generated) copy constructed does not work for "Owned" pointers because the default action is a shallow copy.

Be very careful of RAW pointers.

            sql::Statement *stmt;
            sql::ResultSet *result;

Any assignment to these variables is not exception safe. You will need to take extra precautions that no exceptions are thrown before the delete statements otherwise you will leak. As a result you should probably prefer to use a smart pointer (like you use above).

            stmt = con->createStatement();    
            result = stmt->executeQuery("select * from daily_history where symbol_id = 'AA';");

You better make sure no statements from here throw!

I dislike this.
You are manually opening and closing things. You want to use C++ RAII to do this automatically if you can. This will prevent accidental programer mistakes.

            writer.StartArray();
                while (result->next()) {
                    writer.StartArray();
                        writer.Int(result->getInt(2));       // timestamp
                        writer.Double(result->getDouble(3)); // open
                        writer.Double(result->getDouble(4)); // high
                        writer.Double(result->getDouble(5)); // low
                        writer.Double(result->getDouble(6)); // close
                        writer.Uint(result->getUInt(7));     // volume
                    writer.EndArray();
                }
            writer.EndArray();

Manual delete.

            delete result;
            delete stmt;

Modern C++ see very few manual deletions. RAII allows the management of this to be done automatically.

Manual management of a connection.

            pool->releaseConnection(con);

You are manually doing something that should be automatic. Here you are not going to leak the connection. But if you forget then you are going to loose items from the pool and thus slowly make your pool less efficient.

We are we using dynamically created objects here:

    WebRouter *router = new WebRouter(); 

Much simpler to to use automatic object.

    WebRouter  router;