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
