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.
>
> 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.)
>
>
> 2)
>
> SQLExecutor::SQLExecutor(const string &user, const string &schema)
>
> Please remove the user parameter. We can add it back once we actually
> implement authentication.
>
>
>
>
> 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.
>
>
>
>
> 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.
>
> 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!
>
>
>
> 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.
>
>
>
>
>
>
> 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
