Skip to content

Commit

Permalink
Final update for asnum support, with working tests
Browse files Browse the repository at this point in the history
  • Loading branch information
robbie committed Jan 13, 2016
1 parent 828b30a commit 1d5b0b8
Show file tree
Hide file tree
Showing 8 changed files with 116 additions and 14 deletions.
1 change: 1 addition & 0 deletions AUTHORS
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,4 @@ Lorenzo Pistone <[email protected]>
Mike Trinkala <[email protected]>
Vadim A. Misbakh-Soloviov <[email protected]>
Marcin Deranek <[email protected]>
Robison Jacka <[email protected]>
10 changes: 10 additions & 0 deletions rockspec/lua-geoip-0.1-1.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ build = {
"src/"
},
libraries = { "GeoIP" }
},
["geoip.asnum"] = {
sources = {
"src/database.c",
"src/asnum.c"
},
incdirs = {
"src/"
},
libraries = { "GeoIP" }
}
}
}
10 changes: 10 additions & 0 deletions rockspec/lua-geoip-0.1.1-1.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ build = {
"src/"
},
libraries = { "GeoIP" }
},
["geoip.asnum"] = {
sources = {
"src/database.c",
"src/asnum.c"
},
incdirs = {
"src/"
},
libraries = { "GeoIP" }
}
}
}
10 changes: 10 additions & 0 deletions rockspec/lua-geoip-0.1.2-1.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ build = {
"src/"
},
libraries = { "GeoIP" }
},
["geoip.asnum"] = {
sources = {
"src/database.c",
"src/asnum.c"
},
incdirs = {
"src/"
},
libraries = { "GeoIP" }
}
}
}
10 changes: 10 additions & 0 deletions rockspec/lua-geoip-scm-1.rockspec
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ build = {
"src/"
},
libraries = { "GeoIP" }
},
["geoip.asnum"] = {
sources = {
"src/database.c",
"src/asnum.c"
},
incdirs = {
"src/"
},
libraries = { "GeoIP" }
}
}
}
7 changes: 5 additions & 2 deletions src/asnum.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ static int lasnum_query_by_name(lua_State * L)
{
return lua_error(L); /* Error message already on stack */
}

org = GeoIP_name_by_name_gl(pGeoIP, name, &gl);
lua_pushstring(L, org);
GeoIP_org_by_delete(org);
Expand Down Expand Up @@ -100,6 +101,7 @@ static int lasnum_query_by_ipnum(lua_State * L)
{
return lua_error(L); /* Error message already on stack */
}

org = GeoIP_name_by_ipnum_gl(pGeoIP, ipnum, &gl);
lua_pushstring(L, org);
GeoIP_org_by_delete(org);
Expand Down Expand Up @@ -163,11 +165,12 @@ static int lasnum_tostring(lua_State * L)
return 1;
}

static const luaL_reg M[] =
static const luaL_Reg M[] =
{
{ "query_by_name", lasnum_query_by_name },
{ "query_by_addr", lasnum_query_by_addr },
{ "query_by_ipnum", lasnum_query_by_ipnum },

{ "charset", lasnum_charset },
{ "set_charset", lasnum_set_charset },
{ "close", lasnum_close },
Expand All @@ -192,7 +195,7 @@ static int lasnum_open(lua_State * L)
GEOIP_ASNUM_EDITION,
GEOIP_MEMORY_CACHE,
LUAGEOIP_ASNUM_MT,
GEOIP_INDEX_CACHE, /* not allowed */
0, /* all flags allowed */
2,
allowed_types
);
Expand Down
1 change: 1 addition & 0 deletions src/database.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#define LUAGEOIP_COUNTRY_MT "lua-geoip.db.country"
#define LUAGEOIP_CITY_MT "lua-geoip.db.city"
#define LUAGEOIP_ASNUM_MT "lua-geoip.db.asnum"

int luageoip_common_open_db(
lua_State * L,
Expand Down
81 changes: 69 additions & 12 deletions test/test.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,17 @@

pcall(require, 'luarocks.require')

local math = require 'math'
local socket = require 'socket'

local geoip = require 'geoip'
local geoip_country = require 'geoip.country'
local geoip_city = require 'geoip.city'
local geoip_asnum = require 'geoip.asnum'

local geoip_country_filename = select(1, ...) or "./GeoIP.dat"
local geoip_city_filename = select(2, ...) or "./GeoLiteCity.dat"
local geoip_asnum_filename = select(3, ...) or "./GeoIPASNum.dat"

print("TESTING lua-geoip")
print("")
Expand All @@ -25,11 +28,16 @@ print("VERSION: ", assert(geoip_city._VERSION))
print("DESCRIPTION: ", assert(geoip_city._DESCRIPTION))
print("COPYRIGHT: ", assert(geoip_city._COPYRIGHT))
print("")
print("VERSION: ", assert(geoip_asnum._VERSION))
print("DESCRIPTION: ", assert(geoip_asnum._DESCRIPTION))
print("COPYRIGHT: ", assert(geoip_asnum._COPYRIGHT))
print("")

-- Check that required files exist
-- See README on info on how to get them
assert(io.open(geoip_country_filename, "r")):close()
assert(io.open(geoip_city_filename, "r")):close()
assert(io.open(geoip_asnum_filename, "r")):close()

do
local id = assert(geoip.id_by_code('RU'))
Expand All @@ -53,7 +61,7 @@ do
--assert(geoip_country.open(nil, 2 ^ 10) == nil) -- TODO: This should fail
--assert(geoip_country.open(nil, nil, -1) == nil) -- TODO: This should fail

assert(geoip_country.open(geoip_city_filename) == nil)
assert(geoip_country.open(geoip_asnum_filename) == nil)

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Why?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Testing against a different DB name. Calling geoip_country.open() on an asnum DB should return nil.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Yes, but why did you change the existing check? Wouldn't it be better to add a new one?

end

do
Expand All @@ -65,6 +73,15 @@ do
assert(geoip_city.open(geoip_country_filename) == nil)
end

do
assert(geoip_asnum.open("./BADFILENAME") == nil)

--assert(geoip_asnum.open(nil, 2 ^ 10) == nil) -- TODO: This should fail
--assert(geoip_asnum.open(nil, nil, -1) == nil) -- TODO: This should fail

assert(geoip_asnum.open(geoip_country_filename) == nil)
end

do
local flags =
{
Expand All @@ -77,9 +94,9 @@ do

for _, flag in ipairs(flags) do
if flag ~= geoip.INDEX_CACHE then
assert(geoip_country.open(nil, flag)):close()

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Why?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

GeoIP country DBs support the INDEX_CACHE flag.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

OK, but why did you remove the check?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

I didn't remove the check, I instead re-enabled it. Previous behavior was to not attempt opening the country DB if the flag were INDEX_CACHE. It will now attempt to open the country DB with that flag.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

I must be missing something. Please point me to the line where assert(geoip_country.open(nil, flag)):close() is now called.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

https://github.com/robison/lua-geoip/blob/master/test/test.lua#L97

I was mistaken, I remembered taking a look at this last week and researching the INDEX_CACHE flag. I did not actually remove it.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Please note the first argument is nil in my question and geoip_country_filename in your answer.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Ah, you're correct. In the original test, it would fail outright in my initial testing. I may be doing something incorrectly.

lua: test/test.lua:80: lua-geoip.db.country error: failed to open database file
stack traceback:
[C]: in function 'assert'
test/test.lua:80: in main chunk
[C]: ?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

That's ultimately why I removed that from my version.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

AFAIR, nil means "read files from the default location". Please check if this check works on your system with geoip-database package (or equivalent if you are not on Debian-based) installed and put it back to the code with an appropriate remark in a code comment.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Yes, but what if the delivery mechanism for the database files puts them in a non-default location, or the package management does not set a system-wide default location? The test would still fail - it's very specific to system configuration, and not testing the library.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

It is a feature of maxmind's library, not our wrapper (AFAIR), so, it is not up to lua-geoip to decide.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

This is not lua-geoip deciding, this is simply not requiring a system to be configured in one very specific and optional manner for a test to pass.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 21, 2016

To rephrase: it is a feature of the maxmind's library which we have to test. Regardless, this is something that is not related at all to your feature, so please keep the code intact.

This comment has been minimized.

Copy link
@robison

robison Jan 21, 2016

Owner

I'll readd it to the code. I was waiting to push an update until I had some further feedback from you on the topic of 1d5b0b8#commitcomment-15564486

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 22, 2016

Hope to have a moment today to continue the review. Apologies for a delay.

assert(geoip_country.open(geoip_country_filename, flag)):close()
end
assert(geoip_asnum.open(geoip_asnum_filename, flag)):close()
assert(geoip_city.open(geoip_city_filename, flag)):close()
end
end
Expand All @@ -100,6 +117,14 @@ do
geodb:close()
end

do
local geodb = assert(
geoip_asnum.open(geoip_asnum_filename)
)
geodb:close()
geodb:close()
end

do
local check_country = function(db, method, arg)
local id = assert(db[method](db, arg, "id"))
Expand Down Expand Up @@ -163,16 +188,22 @@ do
end
end

local check_asnum = function(db, method, arg)
local org = assert(db[method](db, arg))
end

local geodb_country = assert(geoip_country.open(geoip_country_filename))
local geodb_city = assert(geoip_city.open(geoip_city_filename))
local geodb_asnum = assert(geoip_asnum.open(geoip_asnum_filename))

local checkers =
{
[geodb_country] = check_country;
[geodb_city] = check_city;
[geodb_asnum] = check_asnum;
}

for _, geodb in ipairs { geodb_country, geodb_city } do
for _, geodb in ipairs { geodb_country, geodb_city, geodb_asnum } do
local checker = checkers[geodb]

checker(geodb, "query_by_name", "google-public-dns-a.google.com")
Expand All @@ -182,6 +213,7 @@ do

geodb_country:close()
geodb_city:close()
geodb_asnum:close()
end

-- TODO: Test two different DBs open in parallel work properly
Expand All @@ -200,12 +232,18 @@ local profiles =
file = geoip_city_filename;
field = "country_code";
};
{
name = "asnum";
module = geoip_asnum;
file = geoip_asnum_filename;
field = "org";
};
}

for i = 1, #profiles do
local p = profiles[i]

local geodb = assert(p.module.open(p.file))
local ok, geodb = pcall(p.module.open, p.file)

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Why?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Wrapping the p.module.open in a pcall so that we can (potentially) catch errors, if testing fails.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Aren't you ignoring errors now instead of catching them?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Yes, this may be an oversight on my part. We can catch them with an ok, geodb check.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

I am still not sure how the previous code failed to check that

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

It did, please excuse my lack of experience with Lua. I'll revert this line to the previous state.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 22, 2016

No problem, thanks!


do
print(p.name, "profiling ipnum queries")
Expand All @@ -214,17 +252,24 @@ for i = 1, #profiles do

local cases = { }
for i = 1, num_queries do
cases[i] = math.random(0x7FFFFFFF)
cases[i] = math.random(0x60FFFFFF)

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Why?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

This was a holdover from my testing - I'm not sure what we want to be testing against multicast IP address ranges.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Okay, but please leave a remark about this in the comments

end

local time_start = socket.gettime()
for i = 1, num_queries do
if i % 1e4 == 0 then
print("#", i, "of", num_queries)
end
local result, err = geodb:query_by_ipnum(cases[i], p.field)
if not result and err ~= "not found" then
error(err)
if p.name ~= "asnum" then
local result, err = geodb:query_by_ipnum(cases[i], p.field)
if not result and err ~= "not found" then
error(err)
end
else
local ok, err = geodb:query_by_ipnum(cases[i])
if not ok and err ~= nil then

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Does it make sense to check for err ~= nil here?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Yes, if we don't get a successful response from the DB ('ok'), and the result isn't nil, I'd think that we might want to dump an error, and not proceed with the test suite.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

And if err is nil, but ok is false, should we continue? Why?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

This is semantically identical to the existing error-handling logic, the only difference is that the asnum DB returns nil instead of "not found" if an entry doesn't exist.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Is there a reason why it doesn't return "not found"?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

"not found" is a construct in the city library, where if the pRecord == null, then the string "not found" is pushed onto the lua_State. asnum.c simply returns a nil if an entry for the query does not exist, since there's no pRecord data structure that needs to be populated.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Wouldn't it be better to keep API consistent here?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Since there is actual string data being returned, it's conceivable that 'not found' could be a part of said string data in the asnum DB. Returning nil is the safer solution, instead of creating the additional and unnecessary complexity of a data structure solely to house the contents of the returned string data, or "not found" if the record is not found.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

In Lua it is an idiom to return nil, error_message to indicate error. There is no need for extra data structures for that. Just return a single value, string, if data is found. Return two values, nil and "not found" if it is not.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

I'll update this. Right now, country.c does not return "not found" - it simply returns lua_error(L). Should I do the same for asnum.c?

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Well, something does (or did) return "not found", judging from the test code. Can you please look it up?

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

country.c does not appear to return "not found" - that is exclusively in city.c. The test checks for an error, and if there is an error (and the error result is NOT "not found"), then dump the error + result.

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Looking at this a bit further, it appears that if a country DB query is not successful, the returned countrycode (in the test suite) is 0, and the error message is nil, instead of "not found".

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 22, 2016

Yeah, this is a defect in country DB. City DB code does it right.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 22, 2016

Created an issue re. this: agladysh#14

error(err)
end
end
end

Expand Down Expand Up @@ -256,9 +301,16 @@ for i = 1, #profiles do
if i % 1e4 == 0 then
print("#", i, "of", num_queries)
end
local result, err = geodb:query_by_name(cases[i], p.field)
if not result and err ~= "not found" then
error(err)
if p.name ~= "asnum" then
local ok, err = geodb:query_by_addr(cases[i], p.field)

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Why by_addr? Used to be by_name

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

It still is - the unified raw change bears this out.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

Are you sure?

if p.name ~= "asnum" then

This comment has been minimized.

Copy link
@robison

robison Jan 20, 2016

Owner

Yes. That specific line checks to see if the test being executed is against a DB type named "asnum". If not, it executes and attempts to index against p.fieldi. This check is done so that we don't attempt to index p.field[i] against asnum DBs, since they return only a string and not a data structure.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 20, 2016

You deleted line geodb:query_by_name, and replaced it with geodb:query_by_addr. geodb:query_by_name is not getting tested now. Am I missing something?

This comment has been minimized.

This comment has been minimized.

Copy link
@agladysh

agladysh Jan 22, 2016

Aha, so is this a bug in the test code? The print above says "profiling addr queries", but name query is actually profiled.

if not ok and err ~= "not found" then
error(err)
end
else
local ok, err = geodb:query_by_addr(cases[i])
if not ok and err ~= nil then
error(err)
end
end
end

Expand All @@ -280,7 +332,12 @@ for i = 1, #profiles do
if i % 50 == 0 then
print("#", i, "of", num_queries)
end
assert(geodb:query_by_name("ya.ru", p.field))

if p.name == "asnum" then
assert(geodb:query_by_name("ya.ru"))
else
assert(geodb:query_by_name("ya.ru", p.field))
end
end

print(
Expand Down

5 comments on commit 1d5b0b8

@agladysh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you changed semantics of the existing tests. Please explain.

@robison
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Semantics of the existing tests did not fit 100% with the asnum output. Instead of returning a data structure like the city/country DBs, the asnum DB returns only a string. We needed to catch some of the tests on asnum so that we weren't attempting to index a string.

@agladysh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still, this doesn't explain why should you change existing checks

@robison
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was attempting to keep with the spirit of the testing suite and have the tests run iteratively, inline.

@agladysh
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB: I am not happy with this commit in general --- it changes too many things at the same time. It is good practice to keep commits concise, without mixing several topics in one and to avoid having feature development history in pull requests.

If it is not too much work, I would appreciate if you re-commit your changes. If not, no problem, I can do this myself when I will have a bit of time.

(I will now get back to reviewing the code .)

Please sign in to comment.