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