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
_______________________________________________
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
