Success of PATCH request dependent on dict iteration order

Bug #561521 reported by James Westby
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Launchpad itself
Fix Released
High
Unassigned
lazr.restful
Fix Released
Medium
Unassigned

Bug Description

Hi,

The success of a PATCH request depends on the ordering of dict iteration on
the host, so is too fragile:

EntryManipulatingResource.applyChanges does:

        # For every field in the schema, see if there's a corresponding
        # field in the changeset.
        # Get the fields ordered by name so that we always evaluate them in
        # the same order. This is needed to predict errors when testing.
        for name, field in getFieldsInOrder(self.entry.schema):
            [...]
            validated_changeset[field] = (name, value)

        for field, (name, value) in validated_changeset.items():
            field.set(self.entry, value)

and so the order that .set is called in depends on the order that the items are
returned.

If you have a mutator for a field, where the mutator checks the value of one
of the other fields then this can dictate whether the operation succeeds.

Imagine:

     foo = <boolean field>
     bar = <another field with mutator>

     def mutator_for_b(self, baz):
          if foo:
              raise Exception()
          bar = baz

then, starting from foo = True, doing a PATCH with

  {'foo' = False, bar = dar}

will perhaps fail.

Apparently the ordering changed between hardy and lucid, meaning that
there is at least one test in the launchpad codebase that fails on the latter
but succeeds on the former.

Changing the code so that the method above instead does

        # For every field in the schema, see if there's a corresponding
        # field in the changeset.
        # Get the fields ordered by name so that we always evaluate them in
        # the same order. This is needed to predict errors when testing.
        for name, field in getFieldsInOrder(self.entry.schema):
            [...]
            validated_changeset.append((field, (name, value)))

        for field, (name, value) in validated_changeset:
            field.set(self.entry, value)

makes it work on lucid, and then throwing a reversed() in the second hunk
makes it fail.

I think that this is the correct solution for the changed ordering, given that it
preserves the careful way the first loop iterates in order. However, I have a
concern that there is a latent bug here where getFieldsInOrder iterates the
fields in the opposite order, in which case you still wouldn't be able to make
the change.

Any ordering where the mutator methods may be run before the plain
assignments will fail in certain conditions such as this. Moving the mutator
methods after the assignments would fix that, but there could still be
ordering constraints between methods, so perhaps a way to declare them
could be added?

Thanks,

James

Revision history for this message
James Westby (james-w) wrote :

lib/lp/registry/tests/../stories/webservice/xx-distribution.txt is the test that fails on lucid.

Thanks,

James

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I have this feeling, though it's very much citation needed, that schema set methods shouldn't depend on the value of other schema items for validation, to avoid exactly this issue and that kind of constraint should be expressed as an invariant. But I could be wrong.

In any case, you've identified a clear bug and a sensible fix.

I will say that the comment is wrong: getFieldsInOrder returns the fields in the order they're defined in the schema, not ordered by name. This is good, it's far more sensible than ordering by name, but I guess the comment should be fixed.

Revision history for this message
Leonard Richardson (leonardr) wrote :

I agree that mutator methods should be designed to be run in any order. If this is truly impossible we can add ordering annotations, but I'm not convinced that's necessary.

I can fairly easily apply the changes in a deterministic order, which will at least make things fail consistently.

Gary Poster (gary)
Changed in lazr.restful:
status: New → Triaged
importance: Undecided → High
importance: High → Medium
Gary Poster (gary)
Changed in launchpad-foundations:
status: New → Triaged
milestone: none → 10.04
importance: Undecided → High
Changed in launchpad-foundations:
status: Triaged → Fix Released
Changed in lazr.restful:
status: Triaged → Fix Released
Changed in launchpad-foundations:
status: Fix Released → Fix Committed
Revision history for this message
Leonard Richardson (leonardr) wrote :

This should be fixed in your local Launchpad install (once you update dependencies). Can you verify this?

Revision history for this message
James Westby (james-w) wrote : Re: [Bug 561521] Re: Success of PATCH request dependent on dict iteration order

On Tue, 27 Apr 2010 13:42:47 -0000, Leonard Richardson <email address hidden> wrote:
> This should be fixed in your local Launchpad install (once you update
> dependencies). Can you verify this?

You can do the same verification I would by running

  lib/lp/registry/tests/../stories/webservice/xx-distribution.txt

on a lucid install.

I won't have time to update LP and test this in the short term.

Thanks,

James

Revision history for this message
Leonard Richardson (leonardr) wrote :

> You can do the same verification I would by running

I have done this, and it worked, but I'm pretty sure it worked on my Lucid install _before_ the change. I'll indicate that the QA is done, but do try it out when you get a chance.

Curtis Hovey (sinzui)
Changed in launchpad-foundations:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.