Ticket #172: Trac172Review.txt

File Trac172Review.txt, 9.1 KB (added by stephen, 9 years ago)

Review of changes

Line 
1Ticket 172: cc channel - use JSON
2
3Reviewing revision 2237
4
5Files modified/added since the branch was created at revision 2101
6(Output of "svn diff -r 2101:HEAD --summarize")
7
8M       src/lib/datasrc/tests/datasrc_unittest.cc
9M       src/lib/datasrc/tests/sqlite3_unittest.cc
10M       src/lib/python/isc/cc/tests/Makefile.am
11A       src/lib/python/isc/cc/tests/message_test.py
12M       src/lib/python/isc/cc/message.py
13M       src/lib/cc/data.h
14M       src/lib/cc/data_unittests.cc
15M       src/lib/cc/session.cc
16M       src/lib/cc/data.cc
17M       src/lib/config/config_data.cc
18M       src/lib/config/tests/ccsession_unittests.cc
19M       src/lib/config/tests/config_data_unittests.cc
20M       src/lib/config/tests/module_spec_unittests.cc
21M       src/lib/config/tests/fake_session.cc
22M       src/lib/config/module_spec.cc
23M       src/lib/config/config_data.h
24M       src/lib/config/module_spec.h
25M       src/lib/config/ccsession.cc
26M       src/bin/auth/auth_srv.cc
27M       src/bin/auth/tests/auth_srv_unittest.cc
28
29src/lib/datasrc/tests/datasrc_unittest.cc
30===
31Changes for this ticket - OK
32
33
34src/lib/datasrc/tests/sqlite3_unittest.cc
35===
36Changes for this ticket - OK
37
38
39src/lib/python/isc/cc/tests/Makefile.am
40===
41Changes for this ticket - OK
42
43Remark: The change commented out the running of data_test.py, session_test.py, and test.py.  Should these files be deleted?
44
45
46src/lib/python/isc/cc/tests/message_test.py
47===
48Not sure about the relevance of the comment "Umz, do we need anything more than abstraction from JSON?"
49
50Should test the behaviour trying to decode a malformed JSON string.
51
52
53src/lib/python/isc/cc/message.py
54===
55Changes for this ticket - OK
56
57
58src/lib/cc/data_unittests.cc
59===
60Test: from_and_to_json
61---
62If "NULL" is accepted as a keyword (see comment about Element::fromJSON in data.cc), the test should include a check for "NULL".
63
64Remark: The comment about returning the same string that was used for creation is not true for doubles, e.g. once a DoubleElement with a value of 0.1 is created, there is no way of telling if it was created with a string of "0.1" or "1e-1".
65
66The tests that create a numeric element from JSON input should also check cases where the exponent is the capital letter "E". 
67
68Should add a tests to cope with the various corner cases mentioned below in the comments for data.cc
69
70
71Test: create_and_value_throws
72---
73In the set of EXPECT_THROW statements at the start of each section of this test, it would be logical to include an "EXPECT_NO_THROW(el->xxxxValue())" to check obtaining a value of the specified type does not thrown an exception.
74
75
76Test: map_element
77---
78The creation of the string long_maptag in this function hides the declaration of the constr string long_maptag in the global namespace. (Although this one is 255 characters long and the global string is 256 in length.)  The global declaration may as well be removed.
79
80
81Test: to_and_from_wire
82---
83Tests should include some invalid strings
84
85
86Test: equals
87---
88There 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)
89
90
91Test: merge
92---
93The tests check that where a and b contain identical tags with different values, the result of merge(a, b) is a map with the common tag containing a value from b.  However, this could be by chance, or because of some rule comparing values (e.g. it might always select the higher value).  Suggest adding a test to check the result of merge(b, a).
94
95All tests in this module are of a map with a single tag/value pair.  Tests should be added to ensure that the merge works correctly when the maps contain multiple tag/value pairs.
96
97
98src/lib/cc/data.h
99===
100Remark: 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?
101
102Comments 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.
103
104The comment "// pure virtuals, every derived class must implement these" is not correct - it is followed by a number of non-pure-virtual definitions.
105
106Remark: 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?
107
108
109src/lib/cc/data.cc
110===
111count_chars_i()
112---
113This will return an incorrect result if the input argument is negative.
114
115
116from_stringstream_number()
117---
118It 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.)
119
120If both "d" and "is_double" are initialized on declaration, so should the other automatic variables.
121
122An 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.
123
124Is the string "1.e2" considered a valid double?  The code will throw a JSON error if this is encountered.
125
126A string of the form "8.  2" will be be interpreted as 8.2
127
128There 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).
129
130
131Element::fromJSON
132---
133The switch statement checks for "t", "T", "f" and "F" to decide whether the element is boolean.  However it only checks for "n" to decide if the element is null.  It should check for "N" as well.
134
135The code is unable to cope with negative numbers (or positive numbers with a leading "+" sign).
136
137
138BoolElement::toJSON
139---
140Remark: IntElement(), DoubleElement() and StringElement() use the <type>Value() (e.g. intValue()) method to access the value stored, whereas this method directly accesses the member variable "b".  This is inconsistent.
141
142
143MapElement::set
144---
145There is a magic number of 255 in the method.  This should be defined as a constant somewhere.
146
147
148src/lib/cc/{data.cc|data.h}
149===
150The following comments apply to these two files generally, and not to the changes that were the subject of this ticket.
151
152Helper functions that are not used outside the module data.cc should be declared "static".
153
154The function char_in seems to be repeating the functionality of the C library function strchr.
155
156Is there really a need for the zero-argument form of "toWire()"?  The implementation (in data.cc) is identical to the implementation of "str()".  Also, it doesn't seem right to have an overloaded method return different things depending on what form is called.
157
158The getValue() methods:  The name implies that these simply return the value and don't alter the contents of the class, so it would be better to declare the methods "const".  The same goes for the type-specific getters and some other methods in derived classes (e.g. MapElement::size())
159
160Would have preferred some documentation for skip_chars - it's not immediately clear what it does.
161
162Class MapElement: a comments in the class declaration asks whether direct iterators should be used instead of exposing the std::map through the call to mapValue.  I would suggest that this would be better; a copy of the map can be obtained via getValue, so there seems to be no need to allow the caller to access the underlying data directly.
163
164Remark: in the longer term, the class Element is designed to hold one of limited number of types.  Would it not be better to use some solution based on boost::variant?
165
166
167src/lib/config/config_data.cc
168===
169Changes for this ticket - OK
170
171
172src/lib/config/config_data.h
173===
174Changes for this ticket - OK
175
176
177src/lib/config/{config_data.cc|config_data.h}
178===
179Remark: There is little internal documentation in this module.  Although the methods in the header file are well-documented, the static methods in the implementation file would benefit from better commenting.
180
181
182src/lib/config/tests/ccsession_unittests.cc
183===
184Changes for this ticket - OK
185
186
187src/lib/config/tests/config_data_unittests.cc
188===
189Changes for this ticket - OK
190
191
192src/lib/config/tests/module_spec_unittests.cc
193===
194Changes for this ticket - OK
195
196
197src/lib/config/tests/fake_session.cc
198===
199Changes for this ticket - OK
200
201
202src/lib/cc/session.cc
203===
204Changes for this ticket - OK
205
206Remark: 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.
207
208
209src/lib/config/module_spec.h
210===
211Changes for this ticket - OK
212
213
214src/lib/config/module_spec.cc
215===
216Changes for this ticket - OK
217
218General: 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.)
219
220
221src/lib/config/ccsession.cc
222===
223Changes for this ticket - OK
224
225
226src/bin/auth/auth_srv.cc
227==
228Changes for this ticket - OK
229
230
231src/bin/auth/tests/auth_srv_unittest.cc
232==
233Changes for this ticket - OK
234