I made the small changes you ask.
Except
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="";
}
Can put it in other function , will easy to track error .
And
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
This one I will do later.
On Fri, Jun 15, 2012 at 4:31 PM, Mohit Srivastava <
srivastavamohi...@gmail.com> wrote:
> 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();
>
> I think , If we put schema , table as constructor argument , then we have
> to specify them as member of class. And that is not needed , they are used
> only as temporary purpose.
>
> --
> Mohit
>
>
>
> On Fri, Jun 15, 2012 at 3:14 PM, Henrik Ingo <henrik.i...@avoinelama.fi>wrote:
>
>> 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
