Is it mgo/txn that is internally unmarahalling onto that? Let's get that fixed at its heart. <div class="gmail_quote">On Jun 8, 2016 12:27 PM, "roger peppe" <<a href="mailto:roger.peppe@canonical.com">roger.peppe@canonical.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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. <a href="https://play.golang.org/p/_1ZPl7iMyn" rel="noreferrer" target="_blank">https://play.golang.org/p/_1ZPl7iMyn</a> On 8 June 2016 at 15:55, Gustavo Niemeyer <<a href="mailto:gustavo.niemeyer@canonical.com">gustavo.niemeyer@canonical.com</a>> 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 <<a href="mailto:rogpeppe@gmail.com">rogpeppe@gmail.com</a>> 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 <<a href="mailto:menno.smits@canonical.com">menno.smits@canonical.com</a>> wrote: >> > >> > >> > On 8 June 2016 at 21:05, Tim Penhey <<a href="mailto:tim.penhey@canonical.com">tim.penhey@canonical.com</a>> 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 >> > <a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a> >> > Modify settings or unsubscribe at: >> > <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" rel="noreferrer" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a> >> > >> >> -- >> Juju-dev mailing list >> <a href="mailto:Juju-dev@lists.ubuntu.com">Juju-dev@lists.ubuntu.com</a> >> Modify settings or unsubscribe at: >> <a href="https://lists.ubuntu.com/mailman/listinfo/juju-dev" rel="noreferrer" target="_blank">https://lists.ubuntu.com/mailman/listinfo/juju-dev</a> > > > > > -- > gustavo @ <a href="http://niemeyer.net" rel="noreferrer" target="_blank">http://niemeyer.net</a> </blockquote></div>