Hi Henrik, http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql/revision/2572 http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql/revision/2571
Please see below: On Fri, Jun 22, 2012 at 1:51 AM, Henrik Ingo <henrik.i...@avoinelama.fi>wrote: > Sorry, > > Please see branch lp:~hingo/drizzle/drizzle-alsosql > > henrik > > On Thu, Jun 21, 2012 at 11:19 PM, Henrik Ingo <henrik.i...@avoinelama.fi> > wrote: > > Hi Mohit > > > > Please see branch TODO > > > > > > > > Sorry, I promised we would be done, but I found a couple things I've > > pointed out in a previous review that are not yet implemented. And > > also a couple new things. Please fix: > > > > > > 0) You haven't done anything with docs/index.rst You need to > > * change documentation since we have changed the structure of the > > json query object. > > Ie. It is now { "query" : { "_id" : 1 } } > > > > * update the version number and changelog. > > > > * also, see next point which needs to be documented too. > > > > As a general rule, make it a habit to change the documentation within > > the same (or at least within the next) commit where you change some > > behavior. > Done > > > > 1) > > > > void 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("`;"); > > } > > > > } > > > > The else branch cannot be default behavior, it is too dangerous. Now > > that you know how to implement options, please add the option > > --json_server.allow_drop_table > > default = false/off > > > > Change the else branch above to: > > if else ( json_server_allow_drop_table == true/on ) > > > > Remember to document the new option in docs/index.rst! (Also, remember > > to document the options to set table and schema, which I'm not yet > > reviewing. > Done > > 2) > > > > SQLExecutor::SQLExecutor(const string &user, const string &schema) > > > > Please remove the user parameter. We can add it back once we actually > > implement authentication. > I didn't change it , may be it helps in future. It wont cause any problem now. > > > > > > > > > > 3) > > > > /** > > * Stores an internal sql query. > > */ > > string _internal_sql_query; > > > > /** > > * Stores a sql string. > > */ > > string _sql; > > > > These are the same. Please remove _internal_sql_query. > > > > _error_type= "sql error"; > > _error_message= _exception.getErrorMessage(); > > _error_code= _exception.getErrorCode(); > > > > These are error variables of json_out. They do not belong in > > SQLExecutor. Here you should just store _exception, please remove > > these lines. Then you can get this information from the exception in > > SQLToJsonGenerator. > Done > > > > > > > > > > 4) It seems to me you do not properly delete objects that were created > > with new operator. In other words, this is now leaking memory! > > > > Any object that is created with "new", must also be freed with > > "delete". For example: > > > > extern "C" void process_api02_json_req(struct evhttp_request *req, void* > ) > > HttpHandler* handler = new HttpHandler(json_out,json_in,req); > > DBAccess* dbAccess = new > > DBAccess(json_in,json_out,req->type,schema,table); > > > > void DBAccess::execute() > > SQLGenerator* generator = new SQLGenerator(_json_in,_schema,_table); > > SQLExecutor* executor = new SQLExecutor("",_schema); > > SQLToJsonGenerator* jsonGenerator = new > > SQLToJsonGenerator(_json_out,_schema,_table,executor); > > > > Alternatively, all of the above cases can actually also be > > instantiated as local variables, ie > > HttpHandler* handler(json_out,json_in,req); > > > > ...in which case a delete isn't needed, rather they are automatically > > deleted when the function ends. > Done > > > > 5) > > bool HttpHandler::validateJson(Json::Reader reader) > > > > The reader object is only needed inside this function. There is no > > need to pass it in from the outside. Please change this method so it > > doesn't take any arguments. > > > > void sendResponse(Json::StyledWriter writer,Json::Value &json_out); > > > > Same is true for the writer here: Just create it inside the function, > > don't pass it as an argument. > > json_out on the other hand is already a member variable of this > > object, you don't need to pass it as an argument, just use _json_out! > yeah you are right but json_out is changing from outside of this class too , That why I passed it into this function. Or another way is use a setter function. > > > > > > > > 6) json_server.cc > > > > table= handler->getTable(); > > if(not table || strcmp(table,"")==0) > > { > > table= default_table.c_str(); > > } > > > > if(table && !strcmp(table,"")==0) > > .... > > else > > { > > handler->generateHttpError(); > > > > This code belongs in HTTPHandler! I would change the name of > > validateJson() to just validate() and put it there. > done > > > > > > > > > > > > > > Ok, when you've done the above: > > > > It seems you didn't yet merge against trunk, which is lp:drizzle. So now: > > > > - please merge my branch above > > - do the changes listed above > > - merge lp:drizzle > > - run make test > > - submit merge request against lp:drizzle > > > > > > Then about comments. I've edited some of the comments and > > documentation. You don't need to work on them any further. Just merge > > my changes, do the changes I've listed above, then do a merge request. > > > > > > I'd like to highlight these changes I've made: > > > > /** > > * a class > > * used to access various members and variables of sql_generator, > > sql_executor, sql_to_json_generator. > > */ > > class DBAccess > > { > > private: > > /** > > * a private variable. > > * stores input json object. > > */ > > > > > > Comments like "a class" or "a private variable" are unnecessary. The > > reader is expected to know that much C++! (An exception is that > > sometimes there is no need to comment on the constructors, in which > > case people write just "Constructor".) > > > > > > The other main thing to note is to start sentences with capital letters. > > > > I made also several other changes which are mostly just a matter of > > style, please check out the diff just as a learning excercise. > > > > > > henrik > > > > > > On Thu, Jun 21, 2012 at 2:07 PM, Mohit Srivastava > > <srivastavamohi...@gmail.com> wrote: > >> Default Schema and table set: > >> > >> > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql/revision/2568 > >> > >> > >> On Tue, Jun 19, 2012 at 9:04 PM, Mohit Srivastava > >> <srivastavamohi...@gmail.com> wrote: > >>> > >>> Hi, > >>> Check this out :) > >>> > >>> > >>> > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2472 > >>> > >>> -- > >>> Mohyt > >>> > >>> > >>> On Mon, Jun 18, 2012 at 1:28 PM, Mohit Srivastava > >>> <srivastavamohi...@gmail.com> wrote: > >>>> > >>>> Hi, > >>>> I get all the point and commit the changes. > >>>> > >>>> > >>>> > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-json_server/revision/2471 > >>>> > >>>> -- > >>>> mohit > >>>> > >>>> On Mon, Jun 18, 2012 at 12:39 PM, Henrik Ingo < > henrik.i...@avoinelama.fi> > >>>> wrote: > >>>>> > >>>>> On Mon, Jun 18, 2012 at 9:21 AM, Mohit Srivastava > >>>>> <srivastavamohi...@gmail.com> wrote: > >>>>> > To GET all details from table: > >>>>> > Now json can be used in two ways: > >>>>> > { "query" : {} , "authentication" : { "username" : "hingo", > "password" > >>>>> > : > >>>>> > "qwerty" } } > >>>>> > or > >>>>> > {"authentication" : { "username" : "hingo", "password" : "qwerty" > } } > >>>>> > > >>>>> > which one we have to prefer. > >>>>> > >>>>> Both. Just like before, it's just that the query part is now embedded > >>>>> into a larger json document. > >>>>> > >>>>> > Also where we have to use authentication , in sql_executor while > >>>>> > creating > >>>>> > session ? > >>>>> > >>>>> Yes. But this is just an example, I don't want to implement > >>>>> authentication yet. the point is just to change the structure of the > >>>>> query json so that it is extensible in the future, such as for use by > >>>>> authentication. > >>>>> > >>>>> > Also , > >>>>> > This particular piece of code is also not needed now. > >>>>> > It gives "_id" in request as higher priority > >>>>> > if ( !_json_in["_id"].asBool() ) > >>>>> > { > >>>>> > if( _id ) > >>>>> > { > >>>>> > _json_in["_id"] = (Json::Value::UInt) atol(_id); > >>>>> > } > >>>>> > } > >>>>> > >>>>> If you remove that code, will this test still pass? > >>>>> > >>>>> 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 > >>>> > >>>> > >>> > >> > > > > > > > > -- > > 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 > > > > -- > 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