Opened 19 months ago

Closed 18 months ago

Last modified 18 months ago

#21126 closed Bug (fixed)

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

Reported by: manfre Owned by: manfre
Component: Database layer (models, ORM) Version: 1.5
Severity: Normal Keywords:
Cc: charettes Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Change History (26)

comment:1 Changed 19 months ago by akaariai

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug

comment:2 Changed 19 months ago by manfre

  • Has patch set
  • Owner changed from nobody to manfre
  • Status changed from new to assigned

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)

comment:3 Changed 19 months ago by charettes

  • Needs tests set
  • Patch needs improvement set

comment:4 Changed 19 months ago by akaariai

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

comment:5 Changed 18 months ago by aaugustin

Why is this a release blocker?

Why does the patch need improvement?

comment:6 Changed 18 months ago by 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.

comment:7 Changed 18 months ago by aaugustin

  • Severity changed from Release blocker to Normal

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.

comment:8 Changed 18 months ago by charettes

  • Cc charettes added

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

comment:9 Changed 18 months ago by akaariai

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.

comment:10 Changed 18 months ago by akaariai

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...

comment:11 Changed 18 months ago by 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.

comment:12 Changed 18 months ago by akaariai

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.

comment:13 Changed 18 months ago by Anssi Kääriäinen <akaariai@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

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.

comment:14 Changed 18 months ago by 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.

comment:15 Changed 18 months ago by 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.

comment:16 Changed 18 months ago by akaariai

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.

comment:17 Changed 18 months ago by 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.

comment:18 Changed 18 months ago by 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.

comment:19 Changed 18 months ago by akaariai

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.

comment:20 Changed 18 months ago by 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."

comment:21 Changed 18 months ago by 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.

comment:22 Changed 18 months ago by 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?

comment:23 Changed 18 months ago by akaariai

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".

comment:24 Changed 18 months ago by akaariai

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

comment:25 Changed 18 months ago by 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.

comment:26 Changed 18 months ago by akaariai

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.

Note: See TracTickets for help on using tickets.
Back to Top