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
