Opened 10 years ago

Closed 9 years ago

#172 closed task (fixed)

cc channel - use JSON

Reported by: larissas Owned by: jelte
Priority: medium Milestone: 05. 3rd Incremental Release: Serious Secondary
Component: ~msgq (obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity:
Sub-Project: Feature Depending on Ticket:
Estimated Difficulty: Add Hours to Ticket:
Total Hours: Internal?:

Description (last modified by larissas)

(at least 2 weeks)

Subtickets

Attachments (1)

Trac172Review.txt (9.1 KB) - added by stephen 9 years ago.
Review of changes

Download all attachments as: .zip

Change History (19)

comment:1 Changed 10 years ago by larissas

  • Description modified (diff)

comment:2 Changed 10 years ago by larissas

--should not introduce additional dependencies

--will require changes througout code tree for config as well (.spec files)

--tests need to be smarter

comment:3 Changed 10 years ago by zhanglikun

  • Component changed from cmd-ctl to msgq

The component should belongs to msgq.

comment:4 Changed 9 years ago by shane

  • Milestone changed from feature backlog item to 05. 3rd Incremental Release
  • Owner set to jelte
  • Status changed from new to assigned

comment:5 Changed 9 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ok, changes in branches/trac172.

For the python side, message.to_wire and from_wire are now simply functions that call json conversion (which is included in python).

For the c++ side, we had three options; Boost has some limited support through property trees to read and write jsons, but property tree do not really map all that well to JSON and the way we use the data. We could have used a JSON library, like json-spirit (which in itself uses boost), but this was essentially what we already had but with a different API. So I decided to simply extend our Element class to do full JSON.

So toJSON(ostream) convers to JSON and fromJSON parses it. .str() and .to_wire() are now essentially the same, but in the future we could add formatting options to the string output.

Since the Element representation already was a subset of JSON, this didn't change much in all the other code (including msgq itself, btw).

Two possible enhancements that are left are to abstract from specific number types until you need them (and make one NumberElement? instead of IntElement? and DoubleElement?, that also has a variable e, so numbers aren't tied to the size of int), and checks that strings only contain the right escapes.

Oh and we should rename the data namespace imo, and perhaps Element and ElementPtr? too, but those changes would be quite simple to do in a separate ticket/branch (easier both to perform and to review if there are no other code changes).

Ready for review.

comment:6 Changed 9 years ago by jelte

Oh and while I was at it I also did ticket #124 here, with the createList() and createMap() functions. I have closed that ticket.

comment:7 Changed 9 years ago by jelte

I added the fix for ticket #250 to this branch as well (updated a line of documentation regarding exceptions in the create() factory functions.

comment:8 Changed 9 years ago by stephen

  • Owner changed from UnAssigned to stephen

Changed 9 years ago by stephen

Review of changes

comment:9 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

comment:10 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Again, the comments that I agreed with and fixed right away are left out in this response, for some I had some extra comments, see below.

src/lib/python/isc/cc/tests/Makefile.am
===
Changes for this ticket - OK

Remark: The change commented out the running of data_test.py, session_test.py, and test.py. Should these files be deleted?

salvaged and readded session_test and data_test, and deleted test.py (which only had wire format tests)

Test: equals
---
There is a comment "why does EXPECT_EQ not work?". EXPECT_EQ seemed to work for me - was it a problem with the version of Google test? (I am using 1.3.0)

indeed, it works here too, changed it back (if it pops up again, we'll know where to look, and EXPECT_EQ does make it much clearer in my opinion)

src/lib/cc/data.h
===
Remark: The choice of constants for the "types" enum: the names "string", "list" and "map" - although aptly named - could be confused with type declarations. Leave for now, but perhaps change in the future?

If you have suggestions, i'd gladly hear them, I also had the plan of renaming the namespace (data), which has a related problem, so we can nicely fit that into one ticket :)

Comments for toWire() method suggest that there ought to be a parameter named omit_length. The comment should have been removed when the method signature was changed.

The comment " pure virtuals, every derived class must implement these" is not correct - it is followed by a number of non-pure-virtual definitions.

Right, i moved another pure virtual there and changed the comment to / \name, so that it is grouped on the same level as the rest.

Remark: The fromJSON() methods include a "throw" in the exception signature which is not the case with other methods (e.g. intValue()). What is the standard for BIND-10 - should method signatures include the list of exceptions that can be thrown?

There is no standard, I suggested this at one point (I think when i wrote the original version of this), but there didn't seem to be much support. I would still like to see it though, but we should do it everywhere or nowhere at all. Nice point for a discussion (and action after it's resolved).

src/lib/cc/data.cc
===
count_chars_i()
---
This will return an incorrect result if the input argument is negative.

ok, added a check, which reverses it and adds 1 to the result (for the -). Only thing where this could fail is the value -0, but i'm not sure whether we should account for this.

from_stringstream_number()
---
It would be consistent to declare d_i inside the "if" clause starting "if (in.peek() == '.') {" as that is the only place it is used. (However, this is a very minor point.)

If both "d" and "is_double" are initialized on declaration, so should the other automatic variables.

An input of something like "1e30" will cause an integer overflow when the statement "i = i * p" is executed. Converting the result to a double should be considered.

That wouldn't solve the general problem, would it? I'm thinking (as said in the comment), that we should consider a general Number class.

Is the string "1.e2" considered a valid double? The code will throw a JSON error if this is encountered.

no, that's invalid

A string of the form "8. 2" will be be interpreted as 8.2

The way I read the JSON spec, that's actually supposed to happen. I noticed that this is a lenience that not all parsers have, so we may reconsider (it's pretty lenient in general, and can recover from certain classes of bad input data)

There is an assumption that the decimal part of a string representing a floating point number has only one digit. This leads to odd results, e.g. "8.23" is interpreted as 10.3 (= 8 + 23/10).

doh, fixed (keep dividing until < 1, i'm thinking there's a more efficient way for this :)

MapElement::set
---
There is a magic number of 255 in the method. This should be defined as a constant somewhere.

Oh, yeah, actually, now that you mention it, that check was imposed by the old wireformat (which had only 1 byte for the tag length), which isn't a problem anymore, so now tags can be as long as you want. Removed the check.

<skipping the more general comments, will handle those later>

src/lib/cc/session.cc
===

Remark: There is little internal documentation in this module. The purpose and implementation need to be documented: the class names suggest that the code could be reused elsewhere, but the lack of documentation will make it difficult to do so.

agree, but out of scope for this ticket. Feel free to create a new one for that.

src/lib/config/module_spec.cc
===

General: this file contains a "getType_string()" function that returns a string representation of the Element::types enum, and a getTypes_value() function that does the inverse. These functions would be better made static methods of the the Element class. (They have got out of sync with the class; there is no translation for the "null" type.)

ok done, renamed them to nameToType and typeToName though

I think that's it for the review, made 4 commits to address your comments:
r2295, r2296, r2297 and r2304

As said above, this does not include that specific set of general comments about both files, I'm not sure whether we shouldn't make a separate task of this (which would be way way smaller than this one)

comment:11 follow-up: Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

src/lib/cc/data.h
Remark: The choice of constants for the "types" enum: the names "string", "list"
and "map" - although aptly named - could be confused with type declarations.
Leave for now, but perhaps change in the future?

If you have suggestions, i'd gladly hear them, I also had the plan of renaming
the namespace (data), which has a related problem, so we can nicely fit that into
one ticket :)

How about STRING, LIST and MAP? (Together with INTEGER, DOUBLE, NONE etc.)

src/lib/cc/data.cc
count_chars_i()
from_stringstream_number()
:

doh, fixed (keep dividing until < 1, i'm thinking there's a more efficient way for this :)

In the code for reading characters from the string, how about reading the next string token from the stream then using boost::lexical_cast to convert it to a number? That way you can attempt to cast it to an int and, if it throws a bad_lexical_cast exception, try casting it to a double.

src/lib/cc/data_unittests.cc
Question about the code being tested: I note that if an object containing (say) the name/value pair '"a" : 1' is merged into an object containing the pair '"a" : 2', the result is an object containing the pair '"a" : 2', i.e. the result is the value from the target. But if it is merged into an object containing '"a" : null', the result does not contain "a" at all. Is this the desired behaviour? (i.e. why does the result not contain '"a" : null'?)

src/lib/config/module_spec.cc

General: this file contains a "getType_string()" function that returns a string
representation of the Element::types enum, and a getTypes_value() function that
does the inverse. These functions would be better made static methods of the the
Element class. (They have got out of sync with the class; there is no translation
for the "null" type.)

ok done, renamed them to nameToType and typeToName though

module_spec.cc still needs to be updated to call them.

comment:12 in reply to: ↑ 11 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

Replying to stephen:

src/lib/cc/data.h
Remark: The choice of constants for the "types" enum: the names "string", "list"
and "map" - although aptly named - could be confused with type declarations.
Leave for now, but perhaps change in the future?

If you have suggestions, i'd gladly hear them, I also had the plan of renaming
the namespace (data), which has a related problem, so we can nicely fit that into
one ticket :)

How about STRING, LIST and MAP? (Together with INTEGER, DOUBLE, NONE etc.)

Let's do that in another task :)

src/lib/cc/data.cc
count_chars_i()
from_stringstream_number()
:

doh, fixed (keep dividing until < 1, i'm thinking there's a more efficient way for this :)

In the code for reading characters from the string, how about reading the next string token from the stream then using boost::lexical_cast to convert it to a number? That way you can attempt to cast it to an int and, if it throws a bad_lexical_cast exception, try casting it to a double.

hmz, relying on exceptions for expected behaviour raises all hairs in my neck. But this code isn't efficient either. Perhaps this is a good example for a micro benchmark.

src/lib/cc/data_unittests.cc
Question about the code being tested: I note that if an object containing (say) the name/value pair '"a" : 1' is merged into an object containing the pair '"a" : 2', the result is an object containing the pair '"a" : 2', i.e. the result is the value from the target. But if it is merged into an object containing '"a" : null', the result does not contain "a" at all. Is this the desired behaviour? (i.e. why does the result not contain '"a" : null'?)

We need a way to remove elements from maps, through hints in the structure itself, setting something to null seemed like a good compromise. I'll update the merge documentation to be clearer about this.

src/lib/config/module_spec.cc

General: this file contains a "getType_string()" function that returns a string
representation of the Element::types enum, and a getTypes_value() function that
does the inverse. These functions would be better made static methods of the the
Element class. (They have got out of sync with the class; there is no translation
for the "null" type.)

ok done, renamed them to nameToType and typeToName though

module_spec.cc still needs to be updated to call them.

doh. r2322

comment:13 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed revision 2340:

1) Review of changes made in latest update
All changes OK.

2) Comment about interpretation of numbers

src/lib/cc/data.cc
count_chars_i()
from_stringstream_number()
:

doh, fixed (keep dividing until < 1, i'm thinking there's a more efficient way for this :)

In the code for reading characters from the string, how about reading the next string
token from the stream then using boost::lexical_cast to convert it to a number? That way
you can attempt to cast it to an int and, if it throws a bad_lexical_cast exception, try
casting it to a double.

hmz, relying on exceptions for expected behaviour raises all hairs in my neck.
But this code isn't efficient either. Perhaps this is a good example for a micro benchmark.

I see what you mean, exceptions should be for unusual cases, not predictable ones. In that case, how about reading the next string token from the stream and trying strtol(): if that has not read all the characters in the token, call strtod()?

3) Additional review point
One other thing related to interpretation of numbers. In my original review I mentioned that Element::fromJSON could not cope with negative numbers (or positive with a leading "+" sign). That was fixed, but checking the code again, it occurs to me that it cannot cope with a decimal number without a leading digit (e.g. ".25").

comment:14 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

ok, reworked the number parsing in r2345

also makes IntElement? use long ints (but still possible to create() with a normal int)

ready for merge?

comment:15 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Reviewed revision 2348. Files modified/added since the last review at revision 2340

M src/lib/cc/data.h
M src/lib/cc/data_unittests.cc
M src/lib/cc/session.cc
M src/lib/cc/data.cc

src/lib/cc/data.h
OK

src/lib/cc/data_unittests.cc
OK

src/lib/cc/data.cc
The check whether strtod() has overflowed should also test the returned value against -HUGE_VAL.

src/lib/cc/session.cc
OK

comment:16 Changed 9 years ago by jelte

  • Owner changed from jelte to stephen

fixed in r2353 (check for -HUGE_VAL + a test for it)

comment:17 Changed 9 years ago by stephen

  • Owner changed from stephen to jelte

Fixes have been checked and all OK - the code can be merged now.

comment:18 Changed 9 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged in r2364, closing ticket.

Note: See TracTickets for help on using tickets.