Ensure contrib (namely comments) is IPv6 capable

[Dan McGee]

This is a follow-on ticket now that #811 has been implemented. What's the design decision on updating django.contrib.comments.models to utilize the new IPv6-capable address field? It appears to be the only core Django application utilizing this field.

It assumes the value of REMOTE_ADDR can be coerced into the field, as you can see in this line from django/contrib/comments/views/comments.py:

comment.ip_address = request.META.get("REMOTE_ADDR", None)

This currently populates PostgreSQL-backed instances correctly if hooked up to an IPv6-capable webserver, but all other databases will fail or truncate (perhaps even silently!) any non-IPv4 value exceeding 15 characters, such as ::ffff:192.168.100.1. The prudent decision to me seems to be to modify the Comment model to use:

ip_address  = models.IPAddressField(_('IP address'), unpack_ipv4=True, blank=True, null=True)

A change like this will require manual database migration for all storage engines that use a char type for this; I'm not sure what your policy (if any) is on that. Either way the end result is no worse than what currently happens with truncation.

[Aymeric Augustin]

We need to handle backwards compatibility carefully here.

I haven't followed closely the implementation of GenericIPAddressField, but I suppose it's not represented like IPAddressField in the database, so your change may break existing users of contrib.comments when they upgrade to 1.4.

If backwards compatibility is broken, detailed instructions must be added to the release notes.

[Sasha Romijn]

It would look like there is a backwards compatibility issue, but I don't believe there is, because the existing code is just as broken as the new code would be when using the old database field.

Regarding storage: both IPAddressField and GenericIPAddressField use either postgres' inet or a char(15)/char(39) for storing. So an IPv4 address stored by IPAddressField can be read by GenericIPAddressField.

Considering the current case:

• Comment comes from an IPv4 address in the current code: no problems
• Comment comes from an IPv6 address in the current code: silently truncated to 15 characters, or complete failure if the database does not do silent truncation (there is no model validation for IPAddressField, so it is not detected as invalid).

In other words: the comment framework is broken when a client comments from an IPv6 address.

Considering this, there is no change in behavior if we use a GenericIPAddressField with a char(15) backing it. IPv4 just works, IPv6 is truncated, either silently or loudly. However, with the unpack_ipv4 parameter, IPv6-mapped IPv4 addresses would work even with a char(15) backend - whereas they break now with IPAddressField.

So, updating this even if the database field is not changed will not introduce any additional failures, and even avoid a failure that currently exists. Obviously the best for the user is to also update the database field appropriately. Therefore, changing the field is not breaking backwards compatibility.

Regarding the patch: I agree this needs tests, I think the field change is fine. REMOTE_ADDR is exactly one of the cases I had in mind when I wrote the unpack_ipv4 parameter.

[Jannis Leidel]

Yeah, definitely one of those dogfood items. +1

[Jacob]

Milestone 1.4 deleted

[Dan McGee]

fix broken code found by changing field type, add release notes

[Dan McGee]

I'm going to throw the magic "data loss" words out there to see if we can get this ticket some attention. :) There is definite data loss for anyone running a Django site over IPv6 using the comments application and not using PostgreSQL- the IP addresses will all be truncated.

Working on adding some tests, but I'm not sure what exactly you guys are looking for here. Given that sqlite just accepts whatever you throw at it, I'm not quite sure where you see tests lacking here and I want to get on the same page.

[Alex Gaynor]

If there's actually dataloss, the test should come in the form of data being sent to the view, and not roundtripping correctly.

[Carl Meyer]

Replying to toofishes:

Working on adding some tests, but I'm not sure what exactly you guys are looking for here. Given that sqlite just accepts whatever you throw at it, I'm not quite sure where you see tests lacking here and I want to get on the same page.

If the current problem only exists on backends that are neither Postgres nor SQLite (because SQLite doesn't enforce max-length, and Postgres has the native inet type), then it's fine to add a test which only fails (given current code) on database backends that exhibit the problem. We do run the tests with ​various database backends, not just SQLite. The test should also have a comment or docstring clarifying under what circumstances it would fail.

Thanks for the report, and your work on this patch!

[Dan McGee]

So would a test change that precedes the field switch be acceptable?

If you run the patch with new tests I just submitted and then run the tests, one of the two new tests fail:

======================================================================
FAIL: testCreateValidCommentIPv6Unpack (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File ".../django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 134, in testCreateValidCommentIPv6Unpack
    self.assertEqual(c.ip_address, address[7:])
AssertionError: u'::ffff:18.52.18.52' != '18.52.18.52'

----------------------------------------------------------------------
Ran 93 tests in 2.719s

FAILED (failures=1)

However, the one you would expect to fail, testCreateValidCommentIPv6, does not, I believe because sqlite does not truncate values even if they are over a column limit. Here is that:

$ sqlite3 test.db
SQLite version 3.7.9 2011-11-01 00:52:41
Enter ".help" for instructions
Enter SQL statements terminated with a ";"
sqlite> create table ( testcol varchar(16) );
Error: near "(": syntax error
sqlite> create table testing ( testcol varchar(16) );
sqlite> insert into testing values ("foobar");
sqlite> insert into testing values ("this is way too long of a value for this column");
sqlite> select * from testing;
foobar
this is way too long of a value for this column

So I'm not sure how to add a positive test showing the existing behavior is broken, as it only breaks with certain DB backends.

I'm typing this and already saw the next comment, so here that is:

$ PYTHONPATH=../ python2 runtests.py --settings=test_mysql comment_tests
Creating test database for alias 'default'...
Got an error creating the test database: (1007, "Can't create database 'test_'; database exists")
Type 'yes' if you would like to try deleting the test database 'test_', or 'no' to cancel: yes
Destroying old test database 'default'...
............................................................../home/dmcgee/projects/django/django/db/backends/mysql/base.py:100: Warning: Data truncated for column 'ip_address' at row 1
  return self.cursor.execute(query, args)
FF.............................
======================================================================
FAIL: testCreateValidCommentIPv6 (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dmcgee/projects/django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 122, in testCreateValidCommentIPv6
    self.assertEqual(c.ip_address, address)
AssertionError: u'2a02::223:6cff:' != '2a02::223:6cff:fe8a:2e8a'

======================================================================
FAIL: testCreateValidCommentIPv6Unpack (regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/dmcgee/projects/django/tests/regressiontests/comment_tests/tests/comment_view_tests.py", line 134, in testCreateValidCommentIPv6Unpack
    self.assertEqual(c.ip_address, address[7:])
AssertionError: u'::ffff:18.52.18' != '18.52.18.52'

----------------------------------------------------------------------
Ran 93 tests in 7.414s

FAILED (failures=2)
Destroying test database for alias 'default'...

So we have a definite breakage in MySQL, it just won't show up elsewhere. This is with the tests from the patch, but before s/IPAddressField/GenericIPAddressField/. After all changes are applied from the patch, all tests pass in MySQL (as well as the default SQLite).

Sorry for the extremely verbose response, but I think we're in good shape here now, outside of any code review or English grammar/phrasing comments.

[Jannis Leidel]

Can you explain the changes made to django/db/models/fields/__init__.py line 1013?
Seems like you drop the verbose_name and name parameters of the GenericIPAddressField field, which it really shouldn't.

[Dan McGee]

added tests with ipv6 address persistence

[Sasha Romijn]

I have a made the ​https://github.com/django/django/pull/819 pull request which updates this field to GenericIPAddressField. I have carefully tested the change proposed here in SQLite, Oracle, MySQL and PostgreSQL. It requires a schema change, added to the 1.6 release notes, but without the schema change behaviour is unaffected so it does not break backwards compatibility.

This is the new and old behaviour when trying to save a comment submitted from an IPv6 address:

Database	Current type	New type	Current situation	With patch, without schema change	With patch and schema change
SQLite	TEXT	TEXT	Stored correctly, although field type is misleading	Stored correctly	Stored correctly
PostgreSQL	INET	INET	Stored correctly, although field type is misleading	Stored correctly	Stored correctly
MySQL	VARCHAR(15)	VARCHAR(39)	Silent truncation	Silent truncation	Stored correctly
Oracle	VARCHAR2(15)	VARCHAR2(39)	Query fails	Query fails	Stored correctly

In other words, when applying the patch without the schema change, commenting from IPv6 can break, but no worse than it does now. Note that running the new regressiontests.comment_tests.tests.comment_view_tests.CommentViewTests.testCreateValidCommentIPv6 without applying the model field change will only trigger a failure on MySQL and Oracle.

[Erik Romijn <eromijn@…>]

In ade992c61e15fbc83d87bfc688c0f844b6ef19fd:

Fixed #16302 -- Ensure contrib.comments is IPv6 capable

Changed the ip_address field for Comment to GenericIPAddressField. Added
instructions to the release notes on how to update the schema of existing
databases.

[Aymeric Augustin <aymeric.augustin@…>]

In 7106a1e594dc28108e7e4f83af3ffb25289d3bb0:

Merge pull request #819 from erikr/master

Fixed #16302 -- Ensured contrib.comments is IPv6 capable.