Issue 11917043: Add get-entries flow, and some tests
|
Created:
10 years, 9 months ago by ekasper Modified:
10 years, 8 months ago CC:
ctlog-opensource-review_google.com Visibility:
Public. |
DescriptionAdd get-entries flow
Patch Set 1 #Patch Set 2 : Add get-entries flow and some tests #Patch Set 3 : format #
Total comments: 12
Patch Set 4 : review comments #
Total comments: 2
Patch Set 5 : distinguish 4xx from 5xx #Patch Set 6 : fix error class #Patch Set 7 : now really fix it #
Total comments: 18
Patch Set 8 : review comments #Patch Set 9 : format #
Total comments: 6
Patch Set 10 : add a test for partial responses #
MessagesTotal messages: 14
This CL adds the basic flow for retrieving entries (but not the decoding of entries just yet). I figured you'd be the best person to review it since you're working on the Java client. Also add tests for the client, using a fake ctlog responder. These tests cover JSON response decoding, as well as client control logic. They do not currently cover handling http(s) correctly.
Sign in to reply to this message.
Some preliminary comments. https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:15: gflags.DEFINE_integer("entry_batch_size", 1000, "Number of entries to fetch " Change the docstring to indicate that this is the maximal number of entries to fetch (the log may have less). https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:36: # requests.models.Response is not easily instantiable locally, so as a(n ugly) That's actually a very valid reason to wrap some library. you could mock the library directly (At least it's possible with pymox, seems like the mocking library you use can do it too) https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:53: def get_json(self, path, params={}): Nit: Name this 'json_from_request' or something similar to indicate that this method is actually making a request (specifically, a get request). https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:61: raise HTTPError("%s returned http error %d: %s" % Slightly misaligned indetation. https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:66: except e: Could this except block not be narrowed down? https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:91: Returns: a ct.proto.client_pb2.SthResponse proto Are you aware of Python's namedtuple module? It's a short and convenient way to define data objects. Since the data is not getting serialized in proto form, it may be an equally-convenient way to define these data structures.
Sign in to reply to this message.
https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:15: gflags.DEFINE_integer("entry_batch_size", 1000, "Number of entries to fetch " On 2013/07/26 16:38:41, Eran wrote: > Change the docstring to indicate that this is the maximal number of entries to > fetch (the log may have less). Done. https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:36: # requests.models.Response is not easily instantiable locally, so as a(n ugly) On 2013/07/26 16:38:41, Eran wrote: > That's actually a very valid reason to wrap some library. you could mock the > library directly (At least it's possible with pymox, seems like the mocking > library you use can do it too) Removed "ugly" :) Ideally I'd like to populate http response content rather than mock specific calls like "json()", however that's not easy to achieve with requests.models.Response. https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:53: def get_json(self, path, params={}): On 2013/07/26 16:38:41, Eran wrote: > Nit: Name this 'json_from_request' or something similar to indicate that this > method is actually making a request (specifically, a get request). Renamed to get_json_response https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:61: raise HTTPError("%s returned http error %d: %s" % On 2013/07/26 16:38:41, Eran wrote: > Slightly misaligned indetation. Done. https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:66: except e: On 2013/07/26 16:38:41, Eran wrote: > Could this except block not be narrowed down? See comment above - it's not raising from its own domain. I see e.g. exceptions from simplejson.decode fall through and I think I've no reasonable way for enumerating all failures or ensuring they stay consistent across versions... https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_cl... src/python/ct/client/log_client.py:91: Returns: a ct.proto.client_pb2.SthResponse proto On 2013/07/26 16:38:41, Eran wrote: > Are you aware of Python's namedtuple module? It's a short and convenient way to > define data objects. Since the data is not getting serialized in proto form, it > may be an equally-convenient way to define these data structures. The data is getting serialized for inclusion in the database.
Sign in to reply to this message.
Friendly ping. Any chance you'll have time to look at this today? On Mon, Jul 29, 2013 at 11:30 AM, <ekasper@google.com> wrote: > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.py<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py> > File src/python/ct/client/log_**client.py (right): > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode15<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode15> > src/python/ct/client/log_**client.py:15: > gflags.DEFINE_integer("entry_**batch_size", 1000, "Number of entries to > fetch " > On 2013/07/26 16:38:41, Eran wrote: > >> Change the docstring to indicate that this is the maximal number of >> > entries to > >> fetch (the log may have less). >> > > Done. > > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode36<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode36> > src/python/ct/client/log_**client.py:36: # requests.models.Response is not > easily instantiable locally, so as a(n ugly) > On 2013/07/26 16:38:41, Eran wrote: > >> That's actually a very valid reason to wrap some library. you could >> > mock the > >> library directly (At least it's possible with pymox, seems like the >> > mocking > >> library you use can do it too) >> > > Removed "ugly" :) > > Ideally I'd like to populate http response content rather than mock > specific calls like "json()", however that's not easy to achieve with > requests.models.Response. > > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode53<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode53> > src/python/ct/client/log_**client.py:53: def get_json(self, path, > params={}): > On 2013/07/26 16:38:41, Eran wrote: > >> Nit: Name this 'json_from_request' or something similar to indicate >> > that this > >> method is actually making a request (specifically, a get request). >> > > Renamed to get_json_response > > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode61<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode61> > src/python/ct/client/log_**client.py:61: raise HTTPError("%s returned http > error %d: %s" % > On 2013/07/26 16:38:41, Eran wrote: > >> Slightly misaligned indetation. >> > > Done. > > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode66<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode66> > src/python/ct/client/log_**client.py:66: except e: > On 2013/07/26 16:38:41, Eran wrote: > >> Could this except block not be narrowed down? >> > > See comment above - it's not raising from its own domain. I see e.g. > exceptions from simplejson.decode fall through and I think I've no > reasonable way for enumerating all failures or ensuring they stay > consistent across versions... > > > https://codereview.appspot.**com/11917043/diff/8001/src/** > python/ct/client/log_client.**py#newcode91<https://codereview.appspot.com/11917043/diff/8001/src/python/ct/client/log_client.py#newcode91> > src/python/ct/client/log_**client.py:91: Returns: a > ct.proto.client_pb2.**SthResponse proto > On 2013/07/26 16:38:41, Eran wrote: > >> Are you aware of Python's namedtuple module? It's a short and >> > convenient way to > >> define data objects. Since the data is not getting serialized in proto >> > form, it > >> may be an equally-convenient way to define these data structures. >> > > The data is getting serialized for inclusion in the database. > > https://codereview.appspot.**com/11917043/<https://codereview.appspot.com/119... >
Sign in to reply to this message.
https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:140: # errors. Actually, we decided to send JSON errors after the fact (including a human readable field) - so do feel free to define appropriate error returns, too. Also, you should be able to tell from the STH whether entries are available...
Sign in to reply to this message.
https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:140: # errors. On 2013/07/30 13:44:42, Ben Laurie (Google) wrote: > Actually, we decided to send JSON errors after the fact (including a human > readable field) - so do feel free to define appropriate error returns, too. Not sure what you mean by "define appropriate error returns"? The RFC says nothing about the JSON format, and this code has to work against any log. (When we raise an HTTPError, response.text does print the JSON contents fwiw.) I've made debugging a bit easier by separating 4xx from 5xx though. > > Also, you should be able to tell from the STH whether entries are available... The log client has no knowledge of an STH - that's why the comment says the caller is responsible for ensuring the range is valid. I've clarified the comment by explicitly saying the caller can do this by fetching an STH first.
Sign in to reply to this message.
On 30 July 2013 15:38, <ekasper@google.com> wrote: > > https://codereview.appspot.**com/11917043/diff/13001/src/** > python/ct/client/log_client.py<https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_client.py> > File src/python/ct/client/log_**client.py (right): > > https://codereview.appspot.**com/11917043/diff/13001/src/** > python/ct/client/log_client.**py#newcode140<https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_client.py#newcode140> > src/python/ct/client/log_**client.py:140: # errors. > On 2013/07/30 13:44:42, Ben Laurie (Google) wrote: > >> Actually, we decided to send JSON errors after the fact (including a >> > human > >> readable field) - so do feel free to define appropriate error returns, >> > too. > > Not sure what you mean by "define appropriate error returns"? The RFC > says nothing about the JSON format, and this code has to work against > any log. > There is nothing to prevent us from improving the protocol, particularly when we find deficiencies like this. This is, after all, an experimental RFC for that very reason. We should perhaps start publishing drafts of an improved protocol. > > (When we raise an HTTPError, response.text does print the JSON contents > fwiw.) > > I've made debugging a bit easier by separating 4xx from 5xx though. > > > > Also, you should be able to tell from the STH whether entries are >> > available... > > The log client has no knowledge of an STH - that's why the comment says > the caller is responsible for ensuring the range is valid. I've > clarified the comment by explicitly saying the caller can do this by > fetching an STH first. > Seems needlessly legalistic - the client clearly can fetch an STH, and could do so in case of an error to clarify why the error occurred. > > https://codereview.appspot.**com/11917043/<https://codereview.appspot.com/119... >
Sign in to reply to this message.
(re-sending with reply-all) On Tue, Jul 30, 2013 at 5:17 PM, Emilia Kasper <ekasper@google.com> wrote: > > > > On Tue, Jul 30, 2013 at 4:44 PM, Ben Laurie <benl@google.com> wrote: > >> >> >> >> On 30 July 2013 15:38, <ekasper@google.com> wrote: >> >>> >>> https://codereview.appspot.**com/11917043/diff/13001/src/** >>> python/ct/client/log_client.py<https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_client.py> >>> File src/python/ct/client/log_**client.py (right): >>> >>> https://codereview.appspot.**com/11917043/diff/13001/src/** >>> python/ct/client/log_client.**py#newcode140<https://codereview.appspot.com/11917043/diff/13001/src/python/ct/client/log_client.py#newcode140> >>> src/python/ct/client/log_**client.py:140: # errors. >>> On 2013/07/30 13:44:42, Ben Laurie (Google) wrote: >>> >>>> Actually, we decided to send JSON errors after the fact (including a >>>> >>> human >>> >>>> readable field) - so do feel free to define appropriate error returns, >>>> >>> too. >>> >>> Not sure what you mean by "define appropriate error returns"? The RFC >>> says nothing about the JSON format, and this code has to work against >>> any log. >>> >> >> There is nothing to prevent us from improving the protocol, particularly >> when we find deficiencies like this. This is, after all, an experimental >> RFC for that very reason. We should perhaps start publishing drafts of an >> improved protocol. >> > > Sure. If there's an improved draft specifying the JSON errors we send then > I can try to improve error handling for logs that support it. (But the JSON > I currently get only contains "success: false" and a human-readable error, > so not sure how you expect me to extract more information out of it?) > > But the code still has to work in general, and in general I can't assume > JSON errors at the moment. > >> >> >>> >>> (When we raise an HTTPError, response.text does print the JSON contents >>> fwiw.) >>> >>> I've made debugging a bit easier by separating 4xx from 5xx though. >>> >>> >>> >>> Also, you should be able to tell from the STH whether entries are >>>> >>> available... >>> >>> The log client has no knowledge of an STH - that's why the comment says >>> the caller is responsible for ensuring the range is valid. I've >>> clarified the comment by explicitly saying the caller can do this by >>> fetching an STH first. >>> >> >> Seems needlessly legalistic - the client clearly can fetch an STH, and >> could do so in case of an error to clarify why the error occurred. >> > > Seems perfectly reasonable to me - this class implements the API calls. > The next layer (in the next CL) implements the strategy for firing out > those calls. > > Also, please keep in mind that we are coding against a potentially > misbehaving log. Thus the implication is only "Got sth with tree size >= N" > => "entries [0, N-1] exist" and never "Got sth with tree size <= N" => > "entries [N, ...] do not exist". > > >>> https://codereview.appspot.**com/11917043/<https://codereview.appspot.com/119... >>> >> >> >
Sign in to reply to this message.
Final comments, code should be good to go after this. Also, +1 to Ben's comment about better defining the errors in the RFC. That would allow separating valid error replies of the log from a misbehaving log returning unexpected replies. I'd expect 4xx errors to be generated by the log (e.g. when the start range is past STH) and 5xx to indicate internal log errors. Either way, if these errors are accurately defined, when an HTTP call fails the translation to a user-facing error would be more straightforward. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:69: error_msg = "%s returned http_error %d: %s" % Braces and indentation: error_msg = ("..." % (url, ...)) https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:149: batch = FLAGS.entry_batch_size Pass in the batch size as an optional method parameter / class parameter. Using flags directly that way makes it difficult to programatically modify them later on. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:159: 'end': min(start+batch-1, end)}) Spaces around the operators. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:180: if not response_size or response_size > expected_response_size: IMHO separating the server response verification from the main flow of this function would add to readability, as currently log reply verification is done in parts: * Checking that response["entries"] exists and can be iterated upon. * Checking that the response size matches the expected response size. * Checking that the format of the data in each entry is valid. I suggest a single method that would check all of the above, possibly returning the valid entries array. That would rid this method of mulitple try/except clauses used for verifying response validity. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:189: entry_response = client_pb2.EntryResponse() Extract a method to translate the json output from the log to an EntryResponse instance. Will make this method shorter. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... File src/python/ct/client/log_client_test.py (right): https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:20: class TrueFakeRequest(log_client.Requester): This name is confusing. How about 'FakeResponder'? https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:98: entries = [entry for entry in client.get_entries(4,4)] entries = list(client.get_entries(4, 4)) https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:145: def test_get_entries_returns_all(self): nit: This test could be unified with test_get_entries. If this test passes, IMHO it provides enough confidence that the case tested in test_get_entries works. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:159: self.assertEqual(10, len(returned_entries)) You have a check for those 10 entries 3 times in 3 test cases, consider extracting a method (every thing past the assignment to returned_entries is shared between these test cases).
Sign in to reply to this message.
Thanks for the comments, I've tried to address them. Also added the new test to Makefile, I'd missed that before. On 2013/07/31 09:22:55, Eran wrote: > Final comments, code should be good to go after this. > Also, +1 to Ben's comment about better defining the errors in the RFC. > That would allow separating valid error replies of the log from a misbehaving > log returning unexpected replies. > I'd expect 4xx errors to be generated by the log (e.g. when the start range is > past STH) and 5xx to indicate internal log errors. > Either way, if these errors are accurately defined, when an HTTP call fails the > translation to a user-facing error would be more straightforward. As we discussed offline, I think the reference client should (for the time being) be able to handle a server that implements the current RFC and nothing more. For the future, I think a decision was made to use JSON. Our server already returns JSON errors but they're no more useful at the moment - I just get "success: false" and a human readable message. We can start a new RFC draft and define better errors, sure. OTOH it's important to keep in mind that we can only trust signed server responses, so I'm not 100% convinced better error codes are immensely useful. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > File src/python/ct/client/log_client.py (right): > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client.py:69: error_msg = "%s returned http_error %d: > %s" % > Braces and indentation: > error_msg = ("..." % > (url, ...)) > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client.py:149: batch = FLAGS.entry_batch_size > Pass in the batch size as an optional method parameter / class parameter. Using > flags directly that way makes it difficult to programatically modify them later > on. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client.py:159: 'end': min(start+batch-1, end)}) > Spaces around the operators. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client.py:180: if not response_size or response_size > > expected_response_size: > IMHO separating the server response verification from the main flow of this > function would add to readability, as currently log reply verification is done > in parts: > * Checking that response["entries"] exists and can be iterated upon. > * Checking that the response size matches the expected response size. > * Checking that the format of the data in each entry is valid. > > I suggest a single method that would check all of the above, possibly returning > the valid entries array. That would rid this method of mulitple try/except > clauses used for verifying response validity. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client.py:189: entry_response = > client_pb2.EntryResponse() > Extract a method to translate the json output from the log to an EntryResponse > instance. > Will make this method shorter. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > File src/python/ct/client/log_client_test.py (right): > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client_test.py:20: class > TrueFakeRequest(log_client.Requester): > This name is confusing. How about 'FakeResponder'? > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client_test.py:98: entries = [entry for entry in > client.get_entries(4,4)] > entries = list(client.get_entries(4, 4)) > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client_test.py:145: def > test_get_entries_returns_all(self): > nit: This test could be unified with test_get_entries. If this test passes, IMHO > it provides enough confidence that the case tested in test_get_entries works. > > https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... > src/python/ct/client/log_client_test.py:159: self.assertEqual(10, > len(returned_entries)) > You have a check for those 10 entries 3 times in 3 test cases, consider > extracting a method (every thing past the assignment to returned_entries is > shared between these test cases).
Sign in to reply to this message.
https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:69: error_msg = "%s returned http_error %d: %s" % On 2013/07/31 09:22:55, Eran wrote: > Braces and indentation: > error_msg = ("..." % > (url, ...)) Done. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:149: batch = FLAGS.entry_batch_size On 2013/07/31 09:22:55, Eran wrote: > Pass in the batch size as an optional method parameter / class parameter. Using > flags directly that way makes it difficult to programatically modify them later > on. Done. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:159: 'end': min(start+batch-1, end)}) On 2013/07/31 09:22:55, Eran wrote: > Spaces around the operators. Style guide says to use my better judgment for spaces around arithmetic operators... but ok, I'll use your judgment instead :) https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:180: if not response_size or response_size > expected_response_size: On 2013/07/31 09:22:55, Eran wrote: > IMHO separating the server response verification from the main flow of this > function would add to readability, as currently log reply verification is done > in parts: > * Checking that response["entries"] exists and can be iterated upon. > * Checking that the response size matches the expected response size. > * Checking that the format of the data in each entry is valid. > > I suggest a single method that would check all of the above, possibly returning > the valid entries array. That would rid this method of mulitple try/except > clauses used for verifying response validity. Agreed, done. I've kept them as object methods (rather than static) to let exceptions simply fall through with all the relevant info. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:189: entry_response = client_pb2.EntryResponse() On 2013/07/31 09:22:55, Eran wrote: > Extract a method to translate the json output from the log to an EntryResponse > instance. > Will make this method shorter. Done. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... File src/python/ct/client/log_client_test.py (right): https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:20: class TrueFakeRequest(log_client.Requester): On 2013/07/31 09:22:55, Eran wrote: > This name is confusing. How about 'FakeResponder'? Sure, done. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:98: entries = [entry for entry in client.get_entries(4,4)] On 2013/07/31 09:22:55, Eran wrote: > entries = list(client.get_entries(4, 4)) Done. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:145: def test_get_entries_returns_all(self): On 2013/07/31 09:22:55, Eran wrote: > nit: This test could be unified with test_get_entries. If this test passes, IMHO > it provides enough confidence that the case tested in test_get_entries works. Removed test_get_entries and and renamed this one to test_get_entries instead. https://codereview.appspot.com/11917043/diff/26001/src/python/ct/client/log_c... src/python/ct/client/log_client_test.py:159: self.assertEqual(10, len(returned_entries)) On 2013/07/31 09:22:55, Eran wrote: > You have a check for those 10 entries 3 times in 3 test cases, consider > extracting a method (every thing past the assignment to returned_entries is > shared between these test cases). Done - nice suggestion, putting verify_entries next to make_entries makes it more obvious what's going on as well.
Sign in to reply to this message.
LGTM except for the loop in log_client.py. Once that's addressed, the code should be good to go. https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:111: request""" Nit: """ in a new line. https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:144: array of entries.""" The word 'array' appears twice, but neededon https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:211: start += len(valid_entries) Correct me if I'm wrong, but the documentation above says the log may return a partial response if end >= tree_size. That would mean this while loop may never terminate if the log returns less valid entries than the requested range (there doesn't seem to be a test case covering this scenario).
Sign in to reply to this message.
https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... File src/python/ct/client/log_client.py (right): https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:111: request""" On 2013/08/06 12:33:14, Eran wrote: > Nit: """ in a new line. Done. https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:144: array of entries.""" On 2013/08/06 12:33:14, Eran wrote: > The word 'array' appears twice, but neededon Done. https://codereview.appspot.com/11917043/diff/37001/src/python/ct/client/log_c... src/python/ct/client/log_client.py:211: start += len(valid_entries) On 2013/08/06 12:33:14, Eran wrote: > Correct me if I'm wrong, but the documentation above says the log may return a > partial response if end >= tree_size. That would mean this while loop may never > terminate if the log returns less valid entries than the requested range (there > doesn't seem to be a test case covering this scenario). In this case, start will go out of range and we'll get an HTTPClientError eventually. I've added a test for this. We also raise when the log returns an empty array, ensuring start is strictly increasing. There is already a test for an empty response, too.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
