Hi Mohit
Great progress, I think we are getting close to finalizing the
DBAccess part. (Then of course need to also work on the HTTP part and
multi-threading.)
Regarding ENUM, I realize now you should just use the one that already
exists in event2/http.h:
enum evhttp_cmd_type {
EVHTTP_REQ_GET = 1 << 0,
EVHTTP_REQ_POST = 1 << 1,
EVHTTP_REQ_HEAD = 1 << 2,
EVHTTP_REQ_PUT = 1 << 3,
EVHTTP_REQ_DELETE = 1 << 4,
EVHTTP_REQ_OPTIONS = 1 << 5,
EVHTTP_REQ_TRACE = 1 << 6,
EVHTTP_REQ_CONNECT = 1 << 7,
EVHTTP_REQ_PATCH = 1 << 8
};
It is already used in json_server.cc, just use it the same way. I'm
also giving some examples below.
Now, to the actual review:
$ ls
DBAccess.cc docs http_handler.h json_server.cc
plugin.ini sql_executor.h sql_generator.h
SQLToJsonGenerator.h
DBAccess.h http_handler.cc json plugin.ac
sql_executor.cc sql_generator.cc SQLToJsonGenerator.cc
Be consistent in file names:
mv SQLToJsonGenerator.* sql_to_json_generator.*
mv DBAccess.* db_access.*
Remember to use "bzr mv"!
DBAccess.h
DBAccess(Json::Value &json_in,Json::Value &json_out);
void execute(const char* type,const char* schema,const char* table);
I think json_in, schema and table belong together. I would change the
above like this:
DBAccess(enum evhttp_cmd_type type,
const char* schema,
const char* table,
Json::Value &json_in,
Json::Value &json_out);
void execute();
DBAccess.cc
I think the code flow here is simple and elegant! The refactoring is
paying off :-)
sql_generator.cc
You can remove this comment on line 102 (it is now done):
// TODO: Need to do json_in[].type() first and juggle it from
there to be safe. See json/value.h
I think you are now not covering the case where client makes a POST
into a table that doesn't exist and it needs to be created?
This should be handled like this:
if req->type == POST
...
executor->executeSQL()
if error and error == table doesn't exist
create table
retry executeSQL()
endif
endif
(For any other errors, return them to client as usual.)
sql_executor.h
SQLExecutor(const string &user, const string &schema);
I would omit user for now, we can put back user and password into the
API once we actually support authentication.
sql_executor.cc
SQLExecutor::SQLExecutor(const string &user, const string &schema)
: _in_error_state(false)
{
/* setup a Session object */
_session= Session::make_shared(plugin::Listen::getNullClient(),
catalog::local());
identifier::user::mptr user_id= identifier::User::make_shared();
user_id->setUser(user);
_session->setUser(user_id);
_session->set_schema(schema);
_result_set= new sql::ResultSet(1);
_sql="";
}
I think the creation of a session needs to be outside of constructor.
The reason is that there could be errors, such as authentication, so
it is easier to handle in the code if it is a separate method and not
the constructor.
Since in this case we really just execute one string of SQL, I propose
you just move the session stuff into executeSQL.
The constructor would then be simply:
SQLExecutor::SQLExecutor(const string &schema)
: _in_error_state(false), _schema(schema), _sql("")
{
}
SQLToJsonGenerator.h
void generateJson(const char* s);
Should be
void generateJson(enum evhttp_cmd_type type);
SQLToJsonGenerator.cc
void SQLToJsonGenerator::generateSQLErrorJson()
{
_json_out["error_type"]= _sql_executor->getErrorType();
_json_out["error_message"]= _sql_executor->getErrorMessage();
_json_out["error_code"]= _sql_executor->getErrorCode();
_json_out["internal_sql_query"]= _sql_executor->getSql();
_json_out["schema"]= _schema;
_json_out["sqlstate"]= _sql_executor->getSqlState();
}
Add also:
_json_out["table"]= _table;
...even if it is redundant now, it will be useful later.
I didn't look at HTTPHandler yet. Will do that separately. Looks
really good now!
henrik
On Thu, Jun 14, 2012 at 12:55 PM, Mohit Srivastava
<srivastavamohi...@gmail.com> wrote:
> Hi Henrik,
>
> I have completed the DBAccess part too, but need to use enum. You might help
> me in that too :)
> These are the new commits , you can find latest code here. Again please
> ignore json_server.cc right now.
>
> Updated plugin.ini , sql_generator & sql_executor:
> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2461
>
> Few changes in sql_generator:
> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2462
>
> New files added:
> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2463
>
> Remove a unwanted file ":w" :
> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2464
>
> Thanks
> --
> Mohit
>
>
> On Thu, Jun 14, 2012 at 12:53 AM, Mohit Srivastava
> <srivastavamohi...@gmail.com> wrote:
>>
>> Hi,
>> You can fond new code here.
>> Please ignore json_server.cc right now , I just use it for manual testing.
>> Now I am on my way to write code for DBAccess.
>>
>>
>> http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2461
>>
>> Suggestions and comments are welcome :)
>>
>> Thanks
>> --
>> Mohit
>
>
--
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
