Is it mgo/txn that is internally unmarahalling onto that?
Let's get that fixed at its heart.
On Jun 8, 2016 12:27 PM, "roger peppe" <roger.pe...@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.nieme...@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 <rogpe...@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.sm...@canonical.com> wrote:
> >> >
> >> >
> >> > On 8 June 2016 at 21:05, Tim Penhey <tim.pen...@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@lists.ubuntu.com
> >> > Modify settings or unsubscribe at:
> >> > https://lists.ubuntu.com/mailman/listinfo/juju-dev
> >> >
> >>
> >> --
> >> Juju-dev mailing list
> >> Juju-dev@lists.ubuntu.com
> >> Modify settings or unsubscribe at:
> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev
> >
> >
> >
> >
> > --
> > gustavo @ http://niemeyer.net
>
--
Juju-dev mailing list
Juju-dev@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/juju-dev
