A cautionary tale - mgo asserts

[Menno Smits]

On 8 June 2016 at 21:05, Tim Penhey <tim.penhey at canonical.com> wrote:

> Hi folks,
>
> tl;dr: not use structs in transaction asserts
>
> ...
>
> The solution is to not use a field struct equality, even though it is easy
> to write, but to use the dotted field notation to check the embedded field
> values.
>


To give a more concrete example, asserting on a embedded document field
like this is problematic:

  ops := []txn.Op{{
      C: "collection",
      Id: ...,
      Assert: bson.D{{"some-field", Thing{A: "foo", B: 99}}},
      Update: ...
  }

Due to the way mgo works[1], the document the transaction operation is
asserting against may have been written with A and B in reverse order, or
the Thing struct in the Assert may have A and B swapped by the time it's
used. Either way, the assertion will fail randomly.

The correct approach is to express the assertion like this:

  ops := []txn.Op{{
      C: "collection",
      Id: ...,
      Assert: bson.D{
          {"some-field.A", "foo"},
          {"some-field.B", 99},
      },
      Update: ...
  }

or this:

  ops := []txn.Op{{
      C: "collection",
      Id: ...,
      Assert: bson.M{
          "some-field.A": "foo",
          "some-field.B": 99,
      },
      Update: ...
  }


> Yet another thing to add to the list of things to check when doing reviews.


I think we can go a bit further and error on attempts to use structs for
comparison in txn.Op asserts in Juju's txn layers in state. Just as we
already do some munging and checking of database operations to ensure
correct multi-model behaviour, we should be able to do this same for this
issue and prevent it from happening again.

- Menno

[1] If transaction operations are loaded and used from the DB (more likely
under load when multiple runners are acting concurrently), the Insert,
Update and Assert fields are loaded as bson.M (this is what the bson
Unmarshaller does for interface{} typed fields). Once this happens field
ordering is lost.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160608/83ad4a9d/attachment.html>