Good. Most are ok. Below are a few remaining comments: Please look at lp:~hingo/drizzle/drizzle-alsosql/ for some doc and code comment changes.
Please merge this before doing more changes, I moved some code around too! On Mon, Jun 25, 2012 at 6:52 AM, Mohit Srivastava <srivastavamohi...@gmail.com> wrote: > Hi Henrik, > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql/revision/2572 > http://bazaar.launchpad.net/~srivastavamohit91/drizzle/drizzle-alsosql/revision/2571 > > On Fri, Jun 22, 2012 at 1:51 AM, Henrik Ingo <henrik.i...@avoinelama.fi> >> > 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 if((not _id || strcmp(_id,"")==0) && _req->type==EVHTTP_REQ_DELETE && !allow_drop_table) { generateDropTableError(); return true; } This is wrong. _id can be given also in query object. Did this pass testing? If yes, please add a test which catches this error. In test driven development approach, you would now: * Add a test case which fails with the above code (e.g. DELETE with _id in query object.) * Fix the code * Run tests again to verify that test now passes. Btw, please note I moved these checks to handler->validate(). I hope the code compiles, didn't check (to save time). Related to these parts of code, could you please also test the following bad input and of course also add a test case for it: POST /json/schema=test&table=test&_id=1 ...with no post data / no query document. >> > 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. No. Realize that in Drizzle we have so called "dead code". It is code that is just lying there even if not used. An example would be code branches to handle views (as opposed to tables) even if Drizzle doesn't support views. The MySQL code base dates back to the 80's and there's lots of old cruft in there. So our mission is to remove dead code, not add more of it. Also, how is this supposed to even work? How could you specify a user but not a password? As I see it you will be forced to change this anyway when we want to implement authentication - it is not useful the way it is now. So either implement real authentication now, and test that it works, or remove the code for 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 This is ok, but it looks like you don't need to make _exception a member variable of SQLToJsonGenerator. Since it is only used in one method, you can instantiate it there: Currently: void SQLToJsonGenerator::generateSQLErrorJson() { _exception= _sql_executor->getException(); _json_out["error_type"]= "sql error"; _json_out["error_message"]= _exception.getErrorMessage(); _json_out["error_code"]= _exception.getErrorCode(); _json_out["internal_sql_query"]= _sql_executor->getSql(); _json_out["schema"]= _schema; _json_out["sqlstate"]= _exception.getSQLState(); _json_out["table"]= _table; } Change it to: void SQLToJsonGenerator::generateSQLErrorJson() { sql::Exception exception= _sql_executor->getException(); _json_out["error_type"]= "sql error"; _json_out["error_message"]= exception.getErrorMessage(); _json_out["error_code"]= exception.getErrorCode(); _json_out["internal_sql_query"]= _sql_executor->getSql(); _json_out["schema"]= _schema; _json_out["sqlstate"]= exception.getSQLState(); _json_out["table"]= _table; } Like a lot of the discussions we've had, this is yet another housekeeping topic: moving all objects and variables to where they properly belong. Here the guiding principle is minimalism: If an object is only needed in one place, declare it there, not in a wider scope. An example of why this is a good principle is that what we have now is actually a source of bugs. Imagine I later add another method SQLToJsonGenerator::foo() which wants to read a value from _exception. Now it is not immediately clear if _exception has been assigned to already, or is null. But with the corrected version there is no _exception member variable on offer, so there is no risk of using it in the wrong place. >> > 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. Yes, but different objects can keep a pointer/reference to json_out and change it when needed. They don't need to pass it as an argument each time that happens. This is the point with member variables: It is passed in with the constructor, and then you can access it whenever needed. If someone else changed it at some point, it doesn't matter. I realize there is an argument for style here: that you are asking handler to send this specific json_out that is passed as an argument. Still, my counterargument is again minimalism: The argument isn't needed, so please remove it. >> > 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 I moved some code from handleRequest() to validate(). Other than that this was ok. 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
