A cautionary tale - mgo asserts

[Gustavo Niemeyer]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ubuntu.com/archives/juju-dev/attachments/20160608/0c96dfa2/attachment.html>