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
