Hi Henrik, Yeah, I remember to add a factory class like dbacess , but that I will do after the completion of SQLToJsonGenerator class.
-- Mohyt On Thu, Jun 7, 2012 at 3:15 AM, Henrik Ingo <henrik.i...@avoinelama.fi>wrote: > 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
