On Wed, Jun 6, 2012 at 4:01 PM, Mohit Srivastava <srivastavamohi...@gmail.com> wrote: > Hi Henrik, > > Here is the code for executor and generator: > > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2460 > > Please review it.
Excellent progress! I'm really glad to see my original code molded into properly factored object-oriented code! Ok, comments are inline with the diff. (CCing drizzle-discuss just for fun.) hingo@mermaid:~/hacking/drizzle/drizzle.bzr/drizzle-json_server-mohit-refactor$ bzr diff -c-1 === modified file 'plugin/json_server/json_server.cc' --- plugin/json_server/json_server.cc 2012-05-26 09:53:57 +0000 +++ plugin/json_server/json_server.cc 2012-06-06 12:49:16 +0000 @@ -69,6 +69,8 @@ #include <drizzled/version.h> #include <plugin/json_server/json/json.h> +#include <plugin/json_server/sql_generator.h> +#include <plugin/json_server/sql_executor.h> These new classes do what they are supposed to, but like I've explained earlier, they should not be included or used directly from the main json_server.cc code. What is missing is a class like DbAccess which will take the json_in object and it will then use SQLGenerator and SQLExecutor. So here you would have just: #include <plugin/json_server/db_access.h> @@ -373,7 +376,15 @@ http_response_code = HTTP_NOTFOUND; http_response_text = "You must specify \"table\" in the request uri query string."; } - else { + else { + + if( schema == NULL || strcmp(schema, "") == 0) + { + schema = "test"; + } + + + // It is allowed to specify _id in the uri and leave it out from the json query. // In that case we put the value from uri into json_in here. // If both are specified, the one existing in json_in wins. (This is still valid, no error.) The if clause is wrongly indented, should be four spaces to align with rest of code. @@ -383,63 +394,34 @@ json_in["_id"] = (Json::Value::UInt) atol(id); } } - - // TODO: In a later stage we'll allow the situation where _id isn't given but some other column for where. - // TODO: Need to do json_in[].type() first and juggle it from there to be safe. See json/value.h - // TODO: Don't SELECT * but only fields given in json query document - char sqlformat[1024];; - char buffer[1024]; - if ( json_in["_id"].asBool() ) - { - // Now we build an SQL query, using _id from json_in - sprintf(sqlformat, "%s", "SELECT * FROM `%s`.`%s` WHERE _id=%i;"); - sprintf(buffer, sqlformat, schema, table, json_in["_id"].asInt()); - } - else { - // If neither _id nor query are given, we return the full table. (No error, maybe this is what you really want? Blame yourself.) - sprintf(sqlformat, "%s", "SELECT * FROM `%s`.`%s`;"); - sprintf(buffer, sqlformat, schema, table); - } - - std::string sql = ""; - sql.append(buffer, strlen(buffer)); - - // We have sql string. Use Execute API to run it and convert results back to JSON. - drizzled::Session::shared_ptr _session= drizzled::Session::make_shared(drizzled::plugin::Listen::getNullClient(), - drizzled::catalog::local()); - drizzled::identifier::user::mptr user_id= identifier::User::make_shared(); - user_id->setUser(""); - _session->setUser(user_id); - //_session->set_schema("test"); - - drizzled::Execute execute(*(_session.get()), true); - - drizzled::sql::ResultSet result_set(1); - - /* Execute wraps the SQL to run within a transaction */ - execute.run(sql, result_set); - drizzled::sql::Exception exception= result_set.getException(); - - drizzled::error_t err= exception.getErrorCode(); - - json_out["sqlstate"]= exception.getSQLState(); - - if ((err != drizzled::EE_OK) && (err != drizzled::ER_EMPTY_QUERY)) - { - json_out["error_type"]="sql error"; - json_out["error_message"]= exception.getErrorMessage(); - json_out["error_code"]= exception.getErrorCode(); - json_out["internal_sql_query"]= sql; - json_out["schema"]= "test"; - } - - while (result_set.next()) + + std::string sql; + + SQLGenerator *sg = new SQLGenerator(json_in,schema,table); + if(sg->generateGetSql()) + sql=sg->getSQL(); + + sql::ResultSet* rs = new sql::ResultSet(1); + SQLExecutor *se = new SQLExecutor("",schema); + if(se->executeSQL(sql)) + rs=se->getResultSet(); + else + { + json_out["error_type"]=se->getErrorType(); + json_out["error_message"]= se->getErrorMessage(); + json_out["error_code"]= se->getErrorCode(); + json_out["internal_sql_query"]= sql; + json_out["schema"]= schema; + } + json_out["sqlstate"]= se->getSqlState(); + + while (rs->next()) { Json::Value json_row; bool got_error = false; - for (size_t x= 0; x < result_set.getMetaData().getColumnCount() && got_error == false; x++) + for (size_t x= 0; x < rs->getMetaData().getColumnCount() && got_error == false; x++) { - if (not result_set.isNull(x)) + if (not rs->isNull(x)) { // The values are now serialized json. We must first // parse them to make them part of this structure, only to immediately @@ -449,14 +431,14 @@ // TODO: Massimo knows of a library to create JSON in streaming mode. Json::Value json_doc; Json::Reader readrow(json_conf); - std::string col_name = result_set.getColumnInfo(x).col_name; - bool r = readrow.parse(result_set.getString(x), json_doc); + std::string col_name = rs->getColumnInfo(x).col_name; + bool r = readrow.parse(rs->getString(x), json_doc); if (r != true) { json_out["error_type"]="json parse error on row value"; json_out["error_internal_sql_column"]=col_name; json_out["error_message"]= reader.getFormatedErrorMessages(); // Just put the string there as it is, better than nothing. - json_row[col_name]= result_set.getString(x); + json_row[col_name]= rs->getString(x); got_error=true; break; } else { json_row[col_name]= json_doc; } } } // When done, append this to result set tree json_out["result_set"].append(json_row); } json_out["query"]= json_in; } I've appended a few lines above that aren't really in the diff. All of the above will go into DbAccess->execute(). Instead of all this code, you should have just 2 lines here: DbAccess *db = new DbAccess(); json_out = db->execute(DbAccess::GET, json_in, schema, table, json_out); Further, I wouldn't just copy all of the above code into DbAccess either. I think you need a third class, call it SQLToJsonGenerator. Then DbAccess would basically consist of this sequence: #include <plugin/json_server/sql_generator.h> #include <plugin/json_server/sql_executor.h> #include <plugin/json_server/sql_to_json_generator.h> boolean DbAccess::execute ( enum DbAccess::http_method m, Json::Value &json_in, string &schema, string &table, Json::Value &json_out) { SQLGenerator calls SQLExecutor calls SQLToJsonGenerator calls return json_out; } I'm not at all sure I got the enum, types or references right in the function declaration, but you should get the idea. (And if you don't I'll think about it in the next review :-) The same should then be done for POST and DELETE methods, I'm omitting those from the email. And of course, there will be lots of opportunity to avoid duplication of the same code. (Ie error handling is pretty much the same for all methods, of course only for GET is there any result set to iterate over.) === modified file 'plugin/json_server/plugin.ini' Ok. Now there will be even more files to add :-) === added file 'plugin/json_server/sql_executor.cc' --- plugin/json_server/sql_executor.cc 1970-01-01 00:00:00 +0000 +++ plugin/json_server/sql_executor.cc 2012-06-06 12:49:16 +0000 <clip> +void SQLExecutor::markInErrorState() + { + _in_error_state= true; + } + + void SQLExecutor::clearErrorState() + { + _in_error_state= false; + } These two function names need to match. I propose setErrorState() / clearErrorState(). The alternative of course is to change the second to clearInErrorState() whcih sounds silly. Btw, in Drizzle I commonly see such simple one liner methods (often accessor methods) written directly into the header file. I don't know why, but it looks like an ok approach, so you could do that too. + const string& SQLExecutor::getErrorMessage() const + { + return _error_message; + } + + const string& SQLExecutor::getErrorType() const + { + return _error_type; + } + + const string& SQLExecutor::getErrorCode() const + { + return _error_code; + } + + const string& SQLExecutor::getInternalSqlQuery() const + { + return _internal_sql_query; + } + + const string& SQLExecutor::getSqlState() const + { + return _sql_state; + } + + sql::ResultSet* SQLExecutor::getResultSet() const + { + return _result_set; + } + So then all of the above can go into the header. + +} /* namespace slave */ +} === added file 'plugin/json_server/sql_executor.h' --- plugin/json_server/sql_executor.h 1970-01-01 00:00:00 +0000 +++ plugin/json_server/sql_executor.h 2012-06-06 12:49:16 +0000 Ok, except the comment above of course applies to this file too. === added file 'plugin/json_server/sql_generator.cc' --- plugin/json_server/sql_generator.cc 1970-01-01 00:00:00 +0000 +++ plugin/json_server/sql_generator.cc 2012-06-06 12:49:16 +0000 @@ -0,0 +1,183 @@ +#include <plugin/json_server/sql_generator.h> +#include <cstdio> + +using namespace std; +namespace drizzle_plugin +{ +namespace json_server +{ + +SQLGenerator::SQLGenerator(const Json::Value json_in,const char* schema , const char* table) +{ + _json_in=json_in; + _sql=""; + _schema=schema; + _table=table; +} + +bool SQLGenerator::generateGetSql() +{ + _sql="SELECT * FROM `"; This function has indentation issues, for instance these first lines only have one space. You should also be consistent within the file, later some functions are indented with 4 spaces. In fact, all files within the plugin must follow same style, so follow json_server.cc. If clause should be indented like this: if ( ... ) { code } + _sql.append(_schema); + _sql.append("`.`"); + _sql.append(_table); + _sql.append("`"); + if ( _json_in["_id"].asBool() ) + { + // Now we build an SQL query, using _id from json_in + _sql.append(" WHERE _id = "); + _sql.append(_json_in["_id"].asString()); + } + _sql.append(";"); + + if(_sql.empty()) + return false; How could _sql ever be empty? You have same check in other functions too. + + return true; +} Btw, Drizzle has this weird coding style that bool functions return false when there is no error and true when there is an error. For instance SQLExecutor::execute(), which is copied, does that. You need to follow this backwards style here too. This applies to all methods in SQLGenerator. The explanation for this weird style is that many C functions return an int, and 0 is the return value for success and non-zero for failure. So the return value shouldn't be thought of as "success", rather "was there an error". +bool SQLGenerator::generateIsTableExistsSql() Should this be IfTableExists ? +{ + _sql="select count(*) from information_schema.tables where table_schema = '"; + _sql.append(_schema); + _sql.append("' AND table_name = '"); + _sql.append(_table); + _sql.append("';"); + + if(_sql.empty()) + return false; + + return true; +} I realize now this is not optimal. We should not query information schema for every single POST request. Instead you should do INSERT first, and if it fails with an error that says table doesn't exist, you should automatically create the table and then re-execute the INSERT. If you want to finish the refactoring first, you can postpone this fix, but please write a TODO: comment as a reminder to do this change later. Swap true/false. + +bool SQLGenerator::generateCreateTableSql() +{ + _sql="COMMIT ;"; + _sql.append("CREATE TABLE "); + _sql.append(_schema); + _sql.append("."); + _sql.append(_table); + _sql.append(" (_id BIGINT PRIMARY KEY auto_increment,"); + // Iterate over json_in keys + Json::Value::Members createKeys(_json_in.getMemberNames() ); + for ( Json::Value::Members::iterator it = createKeys.begin(); it != createKeys.end(); ++it ) + { + const std::string &key = *it; + if(key=="_id") { + continue; + } + _sql.append(key); + _sql.append(" TEXT"); + if( it !=createKeys.end()-1 && key !="_id") + { + _sql.append(","); + } + } + _sql.append(")"); + _sql.append("; "); + + if(_sql.empty()) + return false; + + return true; +} Same for _sql.empty(). Same for true/false. + +bool SQLGenerator::generatePostSql() +{ + _sql="REPLACE INTO `"; + _sql.append(_schema); + _sql.append("`.`"); + _sql.append(_table); + _sql.append("` SET "); + + Json::Value::Members keys( _json_in.getMemberNames() ); + + for ( Json::Value::Members::iterator it = keys.begin(); it != keys.end(); ++it ) + { + if ( it != keys.begin() ) + { + _sql.append(", "); + } + // TODO: Need to do json_in[].type() first and juggle it from there to be safe. See json/value.h + const std::string &key = *it; + _sql.append(key); + _sql.append("="); + Json::StyledWriter writeobject; + switch ( _json_in[key].type() ) + { + case Json::nullValue: + _sql.append("NULL"); + break; + case Json::intValue: + case Json::uintValue: + case Json::realValue: + case Json::booleanValue: + _sql.append(_json_in[key].asString()); + break; + case Json::stringValue: + _sql.append("'\""); + _sql.append(_json_in[key].asString()); + _sql.append("\"'"); + break; + case Json::arrayValue: + case Json::objectValue: + _sql.append("'"); + _sql.append(writeobject.write(_json_in[key])); + _sql.append("'"); + break; + default: + return false; + break; + } + _sql.append(" "); + } + _sql.append(";"); + + if(_sql.empty()) + return false; + + return true; + +} Same comments here. Otherwise looks ok. + +bool SQLGenerator::generateDeleteSql() +{ + if ( _json_in["_id"].asBool() ) + { + _sql= "DELETE FROM `"; + _sql.append(_schema); + _sql.append("`.`"); + _sql.append(_table); + _sql.append("`"); + _sql.append(" WHERE _id = "); + _sql.append(_json_in["_id"].asString()); + _sql.append(";"); + } + else + { + _sql="COMMIT ;"; + _sql.append("DROP TABLE `"); + _sql.append(_schema); + _sql.append("`.`"); + _sql.append(_table); + _sql.append("`;"); + } You need to add a configuration option json_server.json_api_allow_drop_table and only do this if it is set to true/on. + + if(_sql.empty()) + return false; + + return true; + + +} Same comments for the ending. + +const string SQLGenerator::getSQL() const +{ + return _sql; +} + + +} + +} === added file 'plugin/json_server/sql_generator.h' OK. Please work on the DbAccess and SQLToJsonResponse classes and the minor fixes, then we'll review again. This review also reminded me to make a note in our blueprint that the code in SQLGenerator doesn't correctly escape strings nor column names. So for instance if you use an SQL reserved word in a json key name then it will break. We can fix that later, let's continue refactoring first. (And I don't know if I even want to fix it, since it is only a problem as long as we use SQL. A proper fix would be to use storage engine api directly.) henrik -- henrik.i...@avoinelama.fi +358-40-8211286 skype: henrik.ingo irc: hingo www.openlife.cc My LinkedIn profile: http://www.linkedin.com/profile/view?id=9522559 _______________________________________________ Mailing list: https://launchpad.net/~drizzle-discuss Post to : drizzle-discuss@lists.launchpad.net Unsubscribe : https://launchpad.net/~drizzle-discuss More help : https://help.launchpad.net/ListHelp