Support ipaddress objects in "get" method #48

ericpruitt · 2019-07-23T02:58:50Z

This allows the user to provide an IP address in the form of an ipaddress.IPv4Address or ipaddress.IPv6Address as an argument for the "get" method.

oschwald · 2019-07-23T03:11:21Z

For this to be merged, the C extension would have to be updated to support it and there would need to be tests.

ericpruitt · 2019-07-23T04:44:06Z

I'm fine with updating the C extension and tests if the feature is welcomed. I'll follow-up once I've done that.

oschwald · 2019-07-23T14:13:40Z

As long as the feature works on all supported Python versions with both the C extension and pure Python implementation and doesn't affect performance, I'd be happy to include it.

The "test_too_many_get_args" methods was not actually testing what the author intended because unittest.TestCase.assertRaises method accepts multiple function arguments as positional parameters, not tuples as used in the code, so the assertRaises invocation has been updated to correct this mistake.

coveralls · 2019-07-30T01:40:07Z

Coverage increased (+0.4%) to 94.921% when pulling 83832e1 on ericpruitt:ipaddress-arg-1 into cf42a02 on maxmind:master.

coveralls · 2019-07-30T01:40:08Z

Coverage increased (+0.4%) to 94.921% when pulling 332cdd0 on ericpruitt:ipaddress-arg-1 into cf42a02 on maxmind:master.

coveralls · 2019-07-30T01:40:08Z

Coverage increased (+0.4%) to 94.921% when pulling 332cdd0 on ericpruitt:ipaddress-arg-1 into cf42a02 on maxmind:master.

ericpruitt · 2019-07-30T01:45:53Z

The code is ready to be reviewed. This is my first time working on Python C extension code, and I may have not handled the reference counting correctly. I originally had more Py_DECREF calls than I currently do. I initially thought they were correct, but removing them resolved some segfaults I observed running the test suite.

This reverts commit b190d97 because it breaks continuous integration.

The changes the PY_MAJOR_VERSION checks to match the expression used by the maintainers.

ericpruitt · 2019-08-04T22:35:06Z

@oschwald Any idea when someone will be able to review this PR?

ericpruitt · 2019-08-08T15:59:17Z

@rafl or @andyjack maybe?

oschwald · 2019-08-08T22:53:16Z

@ericpruitt, sorry for the delayed response. I am hoping to get to this soon. The C code will need some closer review. I'll add a couple of comments now however based on a cursory review.

oschwald

Some initial comments.

oschwald · 2019-08-08T23:08:36Z

extension/maxminddb.c

+            return NULL;
+        }
+
+        PyObject* ipaddress_IPv4Address = PyDict_GetItemString(


Shouldn't we be using PyObject_IsInstance or something here?

I'm calling PyObject_IsInstance below. PyObject_IsInstance requires a PyObject as its second argument, so I don't understand how I'm supposed to use PyObject_IsInstance without getting the IPv4Address class first.

oschwald · 2019-08-08T23:13:53Z

extension/maxminddb.c

+        if (!is_ipaddress_object) {
+            PyErr_SetString(PyExc_TypeError, "IP address must be a string,"
+                " ipaddress.IPv4Address or ipaddress.IPv6Address object");
+        } else if ((str = PyObject_Str(object)) != NULL) {


It is a little bit weird that we are doing all of these gymnastics and creating these heavy Python string and byte objects. I wonder if we could just get the packed address. Ideally, we wouldn't even bother converting that back to a *char, but it probably doesn't hurt too much to do that conversion.

The only simpler method I could think of would be casting the argument to a str, but then you lose out on type safety.

I wonder if we could just get the packed address

That is exposed:

~$ python Python 3.5.3 (default, Sep 27 2018, 17:25:39) [GCC 6.3.0 20170516] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import ipaddress >>> i = ipaddress.ip_address("127.0.0.1") >>> i.packed b'\x7f\x00\x00\x01'

oschwald · 2019-08-08T23:14:50Z

extension/maxminddb.c

+        } else if ((str = PyObject_Str(object)) != NULL) {
+#if PY_MAJOR_VERSION >= 3
+            bytes = PyUnicode_AsEncodedString(str, "UTF-8", "strict");
+            ip_address = PyBytes_AS_STRING(bytes);


If this code is kept, I think you can just decrement the reference count on bytes here and get rid of the ones below. There are also leaks from early returns as it is done now.

Does Python guarantee that garbage collection will not immediately take place when the reference counter drops to 0? The reason I'm i'm decrementing the reference count in more than one place is because the ip_address pointer points into bytes, and if decrementing the reference counter frees that memory, it introduces a use-after-free bug.

oschwald · 2019-08-08T23:18:21Z

extension/maxminddb.c

    char *ip_address = NULL;
+    PyObject *object = NULL;
+#if PY_MAJOR_VERSION >= 3
+    PyObject *bytes = NULL;


No need to declare the variables at the top of the function.

Where do you propose I declare it? I don't think I can declare it inside the "if" condition since that will cause "bytes" to be scoped to that block which will mean it can't be used in the code further down.

I also can't declare object inside the "if" block because I need to use object in the condition expression.

oschwald · 2019-08-11T22:42:08Z

I had some more time to think about this, and I think it would be cleaner using O& with PyArg_ParseTuple and making a converter function. I made a quick sketch of what this might look like. This is only a sketch and it would need to be updated for older Python versions and cleaned up generally.

Based on my testing, this is also much faster, 30% or so. I assume a lot of that is due to just allowing any object with a packed attribute, although there is less Python object creation generally.

ericpruitt · 2019-08-11T23:10:15Z

Alright, I'll close out this pull request. You probably want to at least pull in the unit test change I made to fix the arguments for assertRaises.

EDIT: I overlooked that this was branched from my pull request, so that change is already included.

oschwald · 2019-09-24T14:57:56Z

#50 adds support for ipaddress objects and will be in the next release.

ericpruitt · 2019-09-26T03:07:17Z

#50 adds support for ipaddress objects and will be in the next release.

Thanks!

ericpruitt added 2 commits July 3, 2019 17:48

Update reader.py

7095aeb

Update reader.py

76b6122

Correct address assignment

a70962c

ericpruitt added 2 commits July 29, 2019 18:32

Support "ipaddress" module objects in extension

332cdd0

Correct indentation of "isinstance" argument

6d2cb82

ericpruitt added 5 commits July 29, 2019 18:49

Remove superfluous space

c101148

Remove submodule changes

b190d97

Revert "Remove submodule changes"

5fbde66

This reverts commit b190d97 because it breaks continuous integration.

Restyle version checking expression

4bd9b2f

The changes the PY_MAJOR_VERSION checks to match the expression used by the maintainers.

Correct reference accounting for "bytes"

83832e1

oschwald requested changes Aug 8, 2019

View reviewed changes

ericpruitt closed this Aug 11, 2019

oschwald mentioned this pull request Sep 20, 2019

Add support for ipaddress objects in "get" method #50

Merged

oschwald mentioned this pull request Oct 3, 2019

Type hints for mypy #45

Closed

Support ipaddress objects in "get" method #48

Support ipaddress objects in "get" method #48

Uh oh!

Conversation

ericpruitt commented Jul 23, 2019

Uh oh!

oschwald commented Jul 23, 2019

Uh oh!

ericpruitt commented Jul 23, 2019

Uh oh!

oschwald commented Jul 23, 2019

Uh oh!

coveralls commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jul 30, 2019

Uh oh!

coveralls commented Jul 30, 2019

Uh oh!

ericpruitt commented Jul 30, 2019

Uh oh!

ericpruitt commented Aug 4, 2019

Uh oh!

ericpruitt commented Aug 8, 2019

Uh oh!

oschwald commented Aug 8, 2019

Uh oh!

oschwald left a comment

Choose a reason for hiding this comment

Uh oh!

oschwald Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschwald Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 9, 2019

Choose a reason for hiding this comment

Uh oh!

oschwald Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschwald Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 8, 2019

Choose a reason for hiding this comment

Uh oh!

ericpruitt Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oschwald commented Aug 11, 2019

Uh oh!

ericpruitt commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oschwald commented Sep 24, 2019

Uh oh!

ericpruitt commented Sep 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

coveralls commented Jul 30, 2019 •

edited

Loading

ericpruitt Aug 8, 2019 •

edited

Loading

ericpruitt Aug 8, 2019 •

edited

Loading

ericpruitt Aug 8, 2019 •

edited

Loading

ericpruitt Aug 8, 2019 •

edited

Loading

ericpruitt commented Aug 11, 2019 •

edited

Loading