GNOME Bugzilla – Bug 696525
geoip-lookup: Use same JSON format as other free services
Last modified: 2013-04-03 17:26:41 UTC
See patch
Created attachment 239710 [details] [review] geoip-lookup: Use same JSON format as other free services While there is no standard on format of JSON strings returned by geoip services, we better use the same format used by existing free services. Since freegeoip.net is the most famous one I know, I followed their format. hostip.info is another that seems to be using the same format[1] as well. This would also mean that apps can easily switch between our service and other. Since our service is not yet running anywhere online, this will also make it easy for any app to use our Ipclient API already w/o having to figure the external IP of the machine they are running on. One thing we do differently than these other services is handling of errors. Both freegeoip.net and hostip.info silently ignore invalid IP and give bugos values while we provide proper errors in the JSON response. This patch also adds some NULL checks. [1] http://www.hostip.info/use.html
Review of attachment 239710 [details] [review]: This looks fine, but it really needs some test files from both services integrated into the test suite, to ensure that we don't get different results based on the service used (as they use the same backend database). ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +16,3 @@ static char *error_message_array [] = { + "Invalid IP address '%s'", + "Can not find the IP address '%s' in the database", This should be a separate patch.
Review of attachment 239710 [details] [review]: Regarding the tests, I agree. It needs to be part of this patch or shall i push this and provide a new patch for that? ::: geocode-glib/geocode-ip-server/geoip-lookup.c @@ +16,3 @@ static char *error_message_array [] = { + "Invalid IP address '%s'", + "Can not find the IP address '%s' in the database", This is part of the changes to print_error_in_json() below.
(In reply to comment #3) > Review of attachment 239710 [details] [review]: > > Regarding the tests, I agree. It needs to be part of this patch or shall i push > this and provide a new patch for that? Provide a separate patch. It's going to be necessary before we commit this though (no tests, no code committed :) > ::: geocode-glib/geocode-ip-server/geoip-lookup.c > @@ +16,3 @@ > static char *error_message_array [] = { > + "Invalid IP address '%s'", > + "Can not find the IP address '%s' in the database", > > This is part of the changes to print_error_in_json() below. Which can be separate.
(In reply to comment #4) > (In reply to comment #3) > > Review of attachment 239710 [details] [review] [details]: > > > > Regarding the tests, I agree. It needs to be part of this patch or shall i push > > this and provide a new patch for that? > > Provide a separate patch. It's going to be necessary before we commit this > though (no tests, no code committed :) Fair enough. :) How about I add that as part of bug#696526 instead as we are only parsing the JSON then? Otherwise I'll have to write a lowlevel test now only to change it later?
You can reuse the JSON parser from bug 696526 for the tests (see geocode-glib-private.h for how we export the other parsers for use in the tests), but it will need to all go in at the same time.
Created attachment 239906 [details] [review] geoip-lookup: Use same JSON format as other free services While there is no standard on format of JSON strings returned by geoip services, we better use the same format used by existing free services. Since freegeoip.net is the most famous one I know, I followed their format. hostip.info is another that seems to be using the same format[1] as well. This would also mean that apps can easily switch between our service and other. Since our service is not yet running anywhere online, this will also make it easy for any app to use our Ipclient API already w/o having to figure the external IP of the machine they are running on. One thing we do differently than these other services is handling of errors. Both freegeoip.net and hostip.info silently ignore invalid IP and give bugos values while we provide proper errors in the JSON response. This patch also adds some NULL checks. [1] http://www.hostip.info/use.html
Created attachment 239907 [details] [review] geoip-lookup: Update error JSON format The JSON format for errors should be the same as normal JSON replies. Since we changed that, we should change this as well.
Created attachment 239908 [details] [review] Add testcase for our & freegeoip JSON formats Is this adequate?
Created attachment 240409 [details] [review] geoip-lookup: Use same JSON format as other free services Rebased on master.
Created attachment 240410 [details] [review] geoip-lookup: Update error JSON format Rebased on master.
Created attachment 240411 [details] [review] Add testcase for our & freegeoip JSON formats Rebased on master.
Review of attachment 240409 [details] [review]: Looks good overall.
Review of attachment 240410 [details] [review]: Looks fine at first glance.
Review of attachment 240411 [details] [review]: ::: geocode-glib/geocode-ip-server/test-geoipformat.c @@ +7,3 @@ +#define ATTRIBUTION "This product includes GeoLite data created by MaxMind, " \ + "available from http://www.maxmind.com\n" +#define OUR_SERVER_RESPONSE "{\"ip\":\"213.243.180.91\"," \ Use on-disk files instead of this horrible format. @@ +48,3 @@ + + g_assert (json_object_has_member (object, "ip")); + g_assert (strcmp (json_object_get_string_member (object, "ip"), "213.243.180.91") == 0); For everything after this, I'd rather: - that you grabbed our ouput from a geoip-server that you would run: $ QUERY_STRING=ip=213.243.180.91 ./geocode-ip-server/geoip-lookup Content-type: text/plain;charset=us-ascii etc. and do a string comparison between the 2. - you compared the outputs of _geocode_ip_json_to_location() for both json results in test-gcglib
Review of attachment 240411 [details] [review]: ::: geocode-glib/geocode-ip-server/test-geoipformat.c @@ +48,3 @@ + + g_assert (json_object_has_member (object, "ip")); + g_assert (strcmp (json_object_get_string_member (object, "ip"), "213.243.180.91") == 0); string comparison? But strings are not the same, only the format is and most of the keys/values.
Created attachment 240441 [details] [review] Add testcase for our & freegeoip JSON formats Acting on the part that I understood and agreed with: Keep test data in files. Since essential client-side patches are merged already and test data is being added by the new patch in bug#696526, this bug now depends on that.
Review of attachment 240411 [details] [review]: ::: geocode-glib/geocode-ip-server/test-geoipformat.c @@ +48,3 @@ + + g_assert (json_object_has_member (object, "ip")); + g_assert (strcmp (json_object_get_string_member (object, "ip"), "213.243.180.91") == 0); And you want me to run the server at runtime from this test binary?
Created attachment 240444 [details] [review] Add testcase for our & freegeoip JSON formats Assuming you asked me to get our server response by launching it at runtime, I've updated the testcase.
Review of attachment 240444 [details] [review]: ::: geocode-glib/geocode-ip-server/Makefile.am @@ +3,3 @@ BUILT_GIRSOURCES = +AM_CFLAGS = -I$(top_srcdir) $(GEOCODE_SERVER_CFLAGS) $(COMMON_CFLAGS) $(WARN_CFLAGS) $(DISABLE_DEPRECATED) -DGEOIP_DATABASE_PATH=\""$(GEOIP_DATABASE_PATH)"\" -DTEST_SRCDIR=\""$(srcdir)/../data/"\" -DSRCDIR=\""$(srcdir)/"\" Looks like it needs reindenting ::: geocode-glib/geocode-ip-server/test-geoipformat.c @@ +79,3 @@ + GError *error = NULL; + + g_assert (g_setenv ("QUERY_STRING", "ip=213.243.180.91", TRUE)); instead of this, you can use: - g_spawn_sync_with_pipes(), so you can pass an environment - g_get_environ(), g_environ_setenv() to set your QUERY_STRING in the env you'll pass. This avoids the env leaking into other tests. @@ +96,3 @@ + g_free (path); + + // Get rid of headers No C++ style comments please @@ +99,3 @@ + p = strstr (standard_output, "\n\n"); + g_assert (p != NULL); + p += 1; +2, to skip the 2 linefeeds, no?
Created attachment 240490 [details] [review] Add testcase for our & freegeoip JSON formats
Created attachment 240494 [details] [review] Add testcase for our & freegeoip JSON formats Forgot to change the C++-style comment.
Review of attachment 240494 [details] [review]: ::: geocode-glib/geocode-ip-server/Makefile.am @@ +10,3 @@ + $(DISABLE_DEPRECATED) \ + -DGEOIP_DATABASE_PATH=\""$(GEOIP_DATABASE_PATH)"\" \ + -DTEST_SRCDIR=\""$(srcdir)/../data/"\" \ $(top_srcdir)/geocode-glib/data/ @@ +11,3 @@ + -DGEOIP_DATABASE_PATH=\""$(GEOIP_DATABASE_PATH)"\" \ + -DTEST_SRCDIR=\""$(srcdir)/../data/"\" \ + -DSRCDIR=\""$(srcdir)/"\" Replace by BUILDDIR and builddir ::: geocode-glib/geocode-ip-server/test-geoipformat.c @@ +83,3 @@ + environ = g_environ_setenv (environ, "QUERY_STRING", "ip=213.243.180.91", TRUE); + + argv[0] = g_build_filename (SRCDIR, "geoip-lookup", NULL); SRCDIR should be BUILDDIR. Otherwise it won't find the compiled binary with srcdir != builddir
Created attachment 240505 [details] [review] Add testcase for our & freegeoip JSON formats
Review of attachment 240505 [details] [review]: Looks good!
Attachment 240409 [details] pushed as 7822e53 - geoip-lookup: Use same JSON format as other free services Attachment 240410 [details] pushed as dbf20e6 - geoip-lookup: Update error JSON format Attachment 240505 [details] pushed as d2216cd - Add testcase for our & freegeoip JSON formats