Code

Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#1186 closed defect (fixed)

[patch] Problem resolving primary key in some kwarg database queries

Reported by: freakboy@… Owned by: adrian
Component: Database layer (models, ORM) Version: magic-removal
Severity: major Keywords: magic-removal kwarg pk primary key query
Cc: Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This patch is provided against magic_removal. The same logic should apply equally well on the trunk.

Queries involving primary keys over a relation currently exhibit some problems. The existing logic in parse_lookup works out if the final query clause is 'pk', and if so, replaces 'pk' with '(primary key)__exact'. This is fine, except that the primary key is determined from the Options object provided to parse_lookup, which is he Options object relating to the main table. Some additional context data is added if the field is a related field.

However, if a custom primary key is in use, and/or the pk query is several levels deep, the primary key is incorrectly resolved, as a different options object is required. If a one-to-one relationship is being used, the extra context is not required.

To overcome these problems, I've done a rework of parse_lookup. The following are the most notable features of the change:

  1. The algorithm is now recursive, rather than iterative. This simplifies the internal logic, and contains less repetition of code (especially with isnull and exact queries). The algorithm is also documented much more extensively than previously.
  2. Primary key lookup behaviour is now determined on the correct Options object
  3. Joins now utilize a 'Sorted Map' data structure, to ensure that joins are applied to the final query in the order that they are constructed.
  4. One-many queries (i.e., reverse lookup over Foreign Keys) are now possible. As well as an Article finding its (single) Reporter, a Reporter can query over all their (many) Articles.
  5. Error messages produced are a little more explicit as to the source of the problem with a kwarg query.

The fix comes as two patches; one to implement the parse_lookup changes, and one to update unit tests. The two patches are almost completely independent.

If you apply the parse_lookup patch without the unit test patch, all existing unit tests will pass, with the exception of two: custom_columns, Line 17 and many_to_one, Line 64. These tests fail due to minor changes in the text of error messages. The same exception is thrown for the same reason, but the error text has changed (as a result of point 4 above). This provides partial proof that the parse_lookup algorithm changes don't change behaviour.

If you apply the unit test patch without the parse_lookup patch, there are 17 failures. 8 of these failures are not really failures in the existing algorithm:

  • 2 failures are due to error text changes
  • 6 failures are due to one-to-many queries not supported in the existing parse_lookup.

However, the remaining 9 failures highlight problems in the existing algorithm:

  • 2 failures are due to custom primary key selection
  • 2 failures are due to primary key selection in many-to-one relations
  • 5 failures highlight problems in one_to_one primary key selection

If you apply both patches, all tests pass.

As an aside, this patch has also adds caching to get_all_related_many_to_many_objects() on Option objects. It uses the same scheme that is used by get_all_related_objects().

Attachments (2)

query-rework.patch (21.8 KB) - added by freakboy@… 8 years ago.
Patch for primary key lookup in parse_lookup
test-query-rework.patch (7.3 KB) - added by freakboy@… 8 years ago.
Unit tests to validate patch for primary key lookup in parse_lookup

Download all attachments as: .zip

Change History (5)

Changed 8 years ago by freakboy@…

Patch for primary key lookup in parse_lookup

Changed 8 years ago by freakboy@…

Unit tests to validate patch for primary key lookup in parse_lookup

comment:1 Changed 8 years ago by adrian

Wow, this is great stuff! Thank you!

comment:2 Changed 8 years ago by adrian

(In [1855]) magic-removal: Changed get_all_related_many_to_many_objects to use caching. Refs #1186. Thanks, Russ

comment:3 Changed 8 years ago by adrian

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

(In [1856]) magic-removal: Fixed #1186 -- Fixed problem resolving primary key in some 'pk' database queries. Also lightly refactored query-parsing code. Thanks, Russ

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.