Issue/#513 New geoip database download URL

[Askaholic]

Updated the geoip url and added a license key parameter. The license key does not have a default and must be set by a server admin in order for the new database to be downloaded.

The database is now served via a tar.gz which contains a copyright and a license text as well as the database. The server now downloads the archive and compares its checksum against the md5 hash available at the same url. Once the archive has been successfully downloaded, the database file is extracted from the archive and copied over the old version of the file (if it exists).

Closes #513

[norraxx]

I just don't get why to use inner functions. Like they're not making code readable, and don't give us much, also they're not using local variables, they're just small functions, that can be extracted out of the class, to utils or something like that. They're just helping functions, that can be separate.

[norraxx]

maybe use https://docs.python.org/3/library/tempfile.html ?

[Askaholic]

I don't see a reason to. The purpose is just so it doesn't overwrite the existing file while downloading. There's no need to use a module just to add /tmp to the beginning of a file path :P

[norraxx]

If something goes wrong, file could stay locked for a while. I just propose the tempfile.

[Askaholic]

How would it get locked? It's opened in a context manager and this function is supposed to be called once every 3 weeks...

[norraxx]

Ok. NP.

[norraxx]

is there a reason for inner function? Like i mean, you don't use local variables from outer scope, etc...

[Askaholic]

Yes, it's for readability. If I put it inline it looks like this:

with tarfile.open(temp_file_path, 'r:gz') as tar:
    for name in tar.getnames():
        if name.endswith("GeoLite2-Country.mmdb"):
            with open(self.file_path, 'wb') as f_out:
                f_in = tar.extractfile(name)

Which is too much indentation IMO

[Wesmania]

We could also do name = next((n for n in tar.getnames() if n.endswith(foo)), None), then throw if it's None.

[Askaholic]

Oh yea, that's pretty clean. There's also a function for getting TarInfo objects instead of just names which we can use to make sure that we only look at files and not directories as well.

[norraxx]

what's the reason behind of it?

[norraxx]

Like, you get_db_member raising if there is no file, isn't it?

[Askaholic]

Because tar.extractfile has return type Optional[IO[bytes]] I get a type error (warning) if I use it directly since the type checker doesn't know about the relationship between tar.getnames and tar.extractfile.

The assert should theoretically always succeed (unless there is a bug in tarfile I guess) but it makes the type checker happy since it basically changes the type from Optional[IO[bytes]] to just IO[bytes].

[norraxx]

=^_^= couldn't see that without analyzer. Yes, i thought about wrong tar too, but i don't know, if there is such possibility, when we already could read name.. like it's highly non possible to not to have whole file, isn't it? :-)

[Askaholic]

Yea, like I said. I don't think it's possible for that assert to fail.

[Wesmania]

Unless someone's feeling malicious and the tar has a directory with GeoLite2-Country.mmdb in it. IMHO it does not hurt to just throw.

[Askaholic]

I did think about whether to move the functions get_db_file_with_checksum and get_checksum somewhere else, but I think it makes more sense to leave them there. They aren't being used anywhere else in the code so I feel that it would just be annoying to read if you had to go find them in a different file. They are basically like lambda functions except that python doesn't allow async lambdas, so they have to be real functions (well coroutines actually).

The other one as I explained is for readability because I didn't want to have 5 levels of nested indentation.

[norraxx]

How about to extract them from class and leave in this file?