On 8 June 2016 at 16:44, Gustavo Niemeyer <[email protected]> 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" <[email protected]> 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 >> <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> >> > >> >> > >> >> > On 8 June 2016 at 21:05, Tim Penhey <[email protected]> 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 >> >> > [email protected] >> >> > Modify settings or unsubscribe at: >> >> > https://lists.ubuntu.com/mailman/listinfo/juju-dev >> >> > >> >> >> >> -- >> >> Juju-dev mailing list >> >> [email protected] >> >> Modify settings or unsubscribe at: >> >> https://lists.ubuntu.com/mailman/listinfo/juju-dev >> > >> > >> > >> > >> > -- >> > gustavo @ http://niemeyer.net -- Juju-dev mailing list [email protected] Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
