Success of PATCH request dependent on dict iteration order
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:
EntryManipulati
# 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 getFieldsInOrde
[...]
for field, (name, value) in validated_
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 getFieldsInOrde
[...]
for field, (name, value) in validated_
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
Changed in lazr.restful: | |
status: | New → Triaged |
importance: | Undecided → High |
importance: | High → Medium |
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 |
Changed in launchpad-foundations: | |
status: | Fix Committed → Fix Released |
lib/lp/ registry/ tests/. ./stories/ webservice/ xx-distribution .txt is the test that fails on lucid.
Thanks,
James