A cautionary tale - mgo asserts

[roger peppe]

On 8 June 2016 at 16:44, Gustavo Niemeyer
<gustavo.niemeyer at canonical.com> wrote:
> Is it mgo/txn that is internally unmarahalling onto that?
>
> Let's get that fixed at its heart.

Yes, good plan.

> On Jun 8, 2016 12:27 PM, "roger peppe" <roger.peppe at canonical.com> wrote:
>>
>> The Assert field in mgo/txn.Op is an interface{}, so
>> when it's marshaled and unmarshaled, the order
>> can change because unmarshaling unmarshals as bson.M
>> which does not preserve key order.
>>
>> https://play.golang.org/p/_1ZPl7iMyn
>>
>> On 8 June 2016 at 15:55, Gustavo Niemeyer
>> <gustavo.niemeyer at canonical.com> wrote:
>> > Is it mgo itself that is changing the order internally?
>> >
>> > It should not do that.
>> >
>> > On Wed, Jun 8, 2016 at 8:00 AM, roger peppe <rogpeppe at gmail.com> wrote:
>> >>
>> >> OK, I understand now, I think.
>> >>
>> >> The underlying problem is that subdocument searches in MongoDB
>> >> are order-sensitive.
>> >>
>> >> For example, I just tried this in a mongo shell:
>> >>
>> >> > db.foo.insert({_id: "one", x: {a: 1, b: 2}})
>> >> > db.foo.find({x: {a: 1, b: 2}})
>> >> { "_id" : "one", "x" : { "a" : 1, "b" : 2 } }
>> >> > db.foo.find({x: {b: 2, a: 1}})
>> >> >
>> >>
>> >> The second find doesn't return anything even though it contains
>> >> the same fields with the same values as the first.
>> >>
>> >> Urk. I did not know about that. What a gotcha!
>> >>
>> >> So it *could* technically be OK if the fields in the struct (and
>> >> any bson.D) are lexically ordered to match the bson Marshaler,
>> >> but well worth avoiding.
>> >>
>> >> I think things would be considerably improved if mgo/bson preserved
>> >> order by default (by using bson.D) when unmarshaling.
>> >> Then at least you'd know that the assertion you specify
>> >> is exactly the one that gets executed.
>> >>
>> >>   cheers,
>> >>     rog.
>> >>
>> >>
>> >>
>> >>
>> >> On 8 June 2016 at 10:42, Menno Smits <menno.smits at canonical.com> wrote:
>> >> >
>> >> >
>> >> > 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.
>> >> >
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Juju-dev mailing list
>> >> > Juju-dev at lists.ubuntu.com
>> >> > Modify settings or unsubscribe at:
>> >> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >> >
>> >>
>> >> --
>> >> Juju-dev mailing list
>> >> Juju-dev at lists.ubuntu.com
>> >> Modify settings or unsubscribe at:
>> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
>> >
>> >
>> >
>> >
>> > --
>> > gustavo @ http://niemeyer.net