Potential data corruption issue with Oracle and Mysql due to SQLCompiler.resolve_columns row, fields misalignment

[Michael Manfre]

When a query is annotated, the aggregate_select values are included in the row, but there are no matching entries in the fields list. This results in the values and fields getting misaligned when attempting to convert the database value to a python value.

Problem can be viewed by adding the following to the top of the for loop in either oracle's or mysql's SQLCompiler.resolve_columns methods:

print '\tfield= %s\tvalue=%s' % (repr(field), value)

Run the test aggregation_regress.AggregationTests.test_more_more and grep for "Peter Norvig" to see that is paired with the "age" column.

Depending on the specific query and data, this can result in the incorrect conversion being applied to the data, which might result in data corruption or an unexpected exception.

[Michael Manfre]

The patches were tested against django-mssql backend. I don't have mysql or oracle configured, so those need to run the 'aggregation_regress' tests.

​https://github.com/django/django/pull/1646 (to master)

​https://github.com/django/django/pull/1647 (to stable/1.5.X)

[Anssi Kääriäinen]

It should be possible to test this with mysql + boolean field in select_related() model.

[Aymeric Augustin]

Why is this a release blocker?

Why does the patch need improvement?

[Michael Manfre]

I marked it as a release blocker due to the potential data corruption issues of running the wrong database converter. If this issue is not severe enough to warrant "release blocker", feel free to downgrade it. It's possible for 3rd party backends to prevent the issue.

I'm guessing charettes marked the patch needing improvement due to the lack of tests, but I expect charettes to clarify that. Anyone with mysql in their environment is more than welcome to write the tests.

[Aymeric Augustin]

Thanks for the clarification. I was asking because this is the only issue that prevents the release of 1.6 RC 1.

Since it isn't a regression, I'm going to downgrade the severity. If a patch with tests is finished quickly enough -- we haven't set a date for RC 1 yet -- it can go in.

[Simon Charette]

@manfre I confirm I marked the patch as needing improvement because it lacks tests which could help spot future regressions.

[Anssi Kääriäinen]

The test would need to have .annotate(someaggregate).select_related(a_model_containing_boolean_field). This should break MySQL's value conversion, that is, the boolean field isn't converted to True/False, instead it remains as the database return value of 1/0.

I can try to write a test tonight, but I can't guarantee anything.

[Anssi Kääriäinen]

Pull request at ​https://github.com/django/django/pull/1678 - master and 1.6.x should be clear cases, the question is if this is safe enough to backpatch. Some 3rd party backends could fail for None field values in convert_values(). Maybe I am just overly cautious here...

[Michael Manfre]

From IRC discussion, a quick check of 3rd party backends shows that the only backends that would (or might) break by the backpatch are django-pyodbc and oracle. I submitted a PR to django-pyodbc to make the backpatch safe. Oracle support is unknown because of issues with testing Oracle with Django 1.5.

[Anssi Kääriäinen]

I added branches for 1.6 and 1.5: ​https://github.com/akaariai/django/compare/ticket_21126_16 and ​https://github.com/akaariai/django/compare/ticket_21126_15.

It would be nice to get a confirmation that Oracle works, at least on 1.5.x as we don't have CI for that combination currently.

[Anssi Kääriäinen <akaariai@…>]

In 83554b018ef283827c0e7459ab934d447b3419d5:

Fixed #21126 -- QuerySet value conversion failure

A .annotate().select_related() query resulted in misaligned rows vs
columns for compiler.resolve_columns() method.

Report & patch by Michael Manfre.

[Anssi Kääriäinen <akaariai@…>]

In d7ae0bc372f8423e7bcf9b5408df46fd5c8dc27d:

[1.6.x] Fixed #21126 -- QuerySet value conversion failure

A .annotate().select_related() query resulted in misaligned rows vs
columns for compiler.resolve_columns() method.

Report & patch by Michael Manfre.

Backpatch of 83554b018ef283827c0e7459ab934d447b3419d5 from master.

[Anssi Kääriäinen <akaariai@…>]

In b7e5b5ba1ed03243f53f9f0bbaa58a33691c815d:

[1.5.x] Fixed #21126 -- QuerySet value conversion failure

A .annotate().select_related() query resulted in misaligned rows vs
columns for compiler.resolve_columns() method.

Report & patch by Michael Manfre.

Backpatch of 83554b018ef283827c0e7459ab934d447b3419d5 from master.

[Anssi Kääriäinen]

I tried to check extra carefully that there is nothing that can possibly break on Oracle. CI should handle master/1.6.x and that should be enough to spot 1.5.x regressions, too. I will run Oracle tests on Friday, I don't have access to Oracle test setup earlier.

[radicalbiscuit]

I'm experiencing this bug or something very similar on Django 1.5 and here's the kicker: I'm using PostgreSQL.

I took a look at the patches, and using the

loaded_fields = self.query.get_loaded_field_names().get(self.query.model, set()) or self.query.select

line for determining aggregate_start completely fixes my issue. It should be noted that the backpatch to 1.5.x did not resolve my issue and, in fact, had no effect, which makes sense since our

The query that triggers this only does so while resolving the aggregates as part of the template layer and includes deferred fields, related fields, and of course, the aggregate.

Can you think of any reason to not included the loaded_fields code in the 1.5 backpatch, and if not, can it be included? Let me know if you need any further information. I'll be keeping an eye on this ticket.

[anonymous]

Whoops, forgot to complete a line:

It should be noted that the backpatch to 1.5.x did not resolve my issue and, in fact, had no effect, which makes sense since resolve_columns is False in this case.

[Anssi Kääriäinen]

As we have already backpatched other changes to that part of compiler, I can't see what we could possibly lose by backpatching a bit more.

[radicalbiscuit <radicalbiscuit@…>]

What would the process on that be? I'm entirely prepared to send in a pull request, but I'm not sure whether to attach it to this issue, reference the other recent patches, or whether it should really be accepted at all. I know it fixes my problem, but I haven't been able to find the cause of my misalignment since I'm using PostgreSQL and resolve_columns is False.

Having said that, I don't see how it would break anything, but I'm not intimately familiar with much of the Django core. And from what I've seen, most patches require greater justification than "Fixed my problem."

[Michael Manfre]

Can you write a test that reproduces the problem or at least provide more details about what specifically in your project is experiencing the error? If you're experiencing the problem with PostgresSQL, then it's probably also experienced by all of the other backends.

[radicalbiscuit <radicalbiscuit@…>]

I can provide a little more information about what we're experiencing. Writing a test that reproduces the error may be possible, but I won't be able to do that just yet.

We dynamically build a query involving non-deferred and deferred fields, select_related fields, and an aggregate (possibly more components, I'd have to dig into that when writing a test).

Inside of the form, the query executes without an issue, but when the code in django.db.models.sql.compiler.SQLCompiler.results_iter() is run as a result of the template layer, the wrong indices are selected for aggregate_start and aggregate_end, such that the wrong elements are selected for processing in resolve_aggregate(). In our specific scenario, this comes to a head when a datetime object is attempted to be cast as an int for a Count aggregate.

The loaded_fields code fixes this issue, I assume because it takes into account non-deferred fields in the presence of deferred fields. What was the original reason for including the loaded_fields distinction? It may help me track down our specific problem. But of more interest to me, why wasn't this particular part backpatched to 1.5?

[Anssi Kääriäinen]

For the backpatching part - Django has a backpatching policy where only critical issues (data loss, security, server crash, bad regressions, ...) get backpatched. The question here is if this issue is severe enough. This ticket got backpatched as data loss issue, but on second thought this might not be actually *that* severe. I guess a different call was made when backpatching was considered for other issues in this area.

I'd like to backpatch rest of the changes in results_iter() to 1.5.x. The method got already changed. Leaving it in a half-fixed situation seems pointless. As long as this ticket isn't used as an example to require more backpatching I can't see any real drawback in doing so. This of course assumes that changes required to 1.5.x are localized to compiler.results_iter(), and are "obviously correct".

[Anssi Kääriäinen]

It seems this is the commit needed to make 1.5 results_iter() fully working: 69f7db153d8108dcef033207d49f4c80febf3d70 / ticket #16436. Can you confirm?

[radicalbiscuit <radicalbiscuit@…>]

That certainly takes care of it for my issue, and that would appear to be the correct ticket and description for my problem. My google-fu was faulty, sensei.

[Anssi Kääriäinen]

I backpatched 69f7db153d8108dcef033207d49f4c80febf3d70 to 1.5.x. Now results_iter() should have all relevant commits backpatched, not just partial backpatch.

I think I made a mistake in the original backpatch of this ticket's patch to 1.5.x. Classifying this as a data loss issue is a bit of a stretch... But as said before, leaving 1.5.x's results_iter() in half-fixed state seems to be the worst option here.