Code

Opened 7 months ago

Closed 7 months ago

Last modified 6 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.

Attachments (0)

Change History (26)

comment:1 Changed 7 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 7 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 7 months ago by charettes

  • Needs tests set
  • Patch needs improvement set

comment:4 Changed 7 months ago by akaariai

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

comment:5 Changed 7 months ago by aaugustin

Why is this a release blocker?

Why does the patch need improvement?

comment:6 Changed 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 7 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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 6 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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.