Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds ability to pass a file descriptor when opening a new Reader() object #33

Merged
merged 3 commits into from
May 11, 2018

Conversation

nkinkade
Copy link
Contributor

Hello!

This PR adds the ability to pass a file descriptor (or really any file-like object) to open_database().

The general use case for these changes is situations where the MaxMind database is not located on the local filesystem, and the database has to be fetched from some remote resource. In these cases, one might not be able to pass a string file path to open_database().

The specific use case that prompted these changes is our desire to use the GeoLite2-City database in a Google App Engine environment. The standard GAE environment does not allow static files larger than 32MB to be included in the app's codebase, which gets pushed to GAE. The GeoLite2-City database file is currently around 55MB. To get around this, we could store the file in Google Cloud Storage, or could even fetch it directly from maxmind.com at runtime. To compound the issue, the GAE environment does not have a wriable filesystem, so we can't just download the file, then pass the local path to open_database(). What we can do, however, is to read the database across the network into a file-like object, then pass that to open_database().

Regarding unit testing, what I've done is a little convoluted. I went this route, though, because I noticed that the current testing runs every test in BaseTestReader() for every possible mode. Since the majority of tests include passing a string path to open_database(), and since I wanted to keep the same testing patterns, I opted to bring in mock to patch the open_database() function as needed. This may be overkill and ugly. A simpler alternative might be something like:

class TestFDReader(unittest.TestCase):
    mode = MODE_FD
    readerClass = [maxminddb.reader.Reader]

    def test_mode_fd_returns_valid_reader(self):
        with open(<test_db_path>) as mmdb_fd:
            reader = open_database(mmdb_fd, mode)
            self.assertIsInstance(reader, maxminddb.reader.Reader)

Any feedback or thoughts on these changes?

Thanks,

Nathan

@coveralls
Copy link

coveralls commented May 11, 2018

Coverage Status

Coverage increased (+0.1%) to 95.192% when pulling 05c95bd on nkinkade:file-descriptor into 893403f on maxmind:master.

@nkinkade
Copy link
Contributor Author

Ugh. I see Python 3.x tests are broken now. Looking into it now.

Copy link
Member

@oschwald oschwald left a comment

Choose a reason for hiding this comment

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

Looks great! 💯 I just have one small request on the documentation of the new mode.

elif mode == MODE_FD:
self._buffer = database.read()
self._buffer_size = len(self._buffer)
filename = database.name
Copy link
Member

Choose a reason for hiding this comment

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

Could you document that the caller is responsible for closing the file descriptor and that it may be closed immediately after creation of the reader?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, and done! Since the various modes are briefly described in numerous places/files, I elected to add this bit of documentation to README.rst. Does this seem sufficient?

Also, if this PR gets merged, I've got another small one for the GeoIP2-python module to enable this feature in that wrapper as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that seems fine for now. Thanks!

… ultimately the caller's responsibility to be sure the file gets closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants