Uploaded image for project: 'C# Driver'
  1. C# Driver
  2. CSHARP-2660

Unexpected behaviour with a new BsonMemberMap instance for SetExtraElementsMember

    • Type: Icon: Bug Bug
    • Resolution: Unresolved
    • Priority: Icon: Major - P3 Major - P3
    • None
    • Affects Version/s: 2.8.1
    • Component/s: Serialization
    • Labels:
      None
    • Minor Change

      There is a subtle bug with the handling of `BsonClassMap.SetExtraElementsMember()`. That method will take any instance of a `BsonMemberMap` but serialization breaks in an unexpected way if you do give it a new instance (and only when there is an extra field to utilize the extra elements member map).

      There is an internal property on `BsonClassMap` called `ExtraElementsMemberMapIndex` which is set from the `Freeze` method on the class map (line 593 of `BsonClassMap`). The comparison performed here is `==` which, as there is no overridden equals method on the class map, uses the default `object.Equals` which is a reference comparison.

      BsonClassMap.cs

      Unable to find source-code formatter for language: csharp. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
      _extraElementsMemberIndex = -1;
      for (int memberIndex = 0; memberIndex < _allMemberMaps.Count; ++memberIndex)
      {
          var memberMap = _allMemberMaps[memberIndex];
          
          // Snip...
      
          if (memberMap == _extraElementsMemberMap)
          {
              _extraElementsMemberIndex = memberIndex;
          }
      }
      

      That reference comparison obviously fails as its a new instance but we only get an error during serialization and the error during serialization is somewhat confusing. It states, when there is an extra field, that the extra field doesn't exist on the deserialization target. But because you've called `SetExtraElementsMember`, you're none-the-wiser as to why. (I only realised it when going through the source code of `BsonClassMap`.)

      BsonClassMapSerializer.cs

      Unable to find source-code formatter for language: csharp. Available languages are: actionscript, ada, applescript, bash, c, c#, c++, cpp, css, erlang, go, groovy, haskell, html, java, javascript, js, json, lua, none, nyan, objc, perl, php, python, r, rainbow, ruby, scala, sh, sql, swift, visualbasic, xml, yaml
      if (extraElementsMemberMapIndex >= 0)
      {
          // Snip...
      }
      else if (_classMap.IgnoreExtraElements)
      {
          bsonReader.SkipValue();
      }
      else
      {
          var message = string.Format(
              "Element '{0}' does not match any field or property of class {1}.",
              elementName, _classMap.ClassType.FullName);
          throw new FormatException(message);
      }
      

      Basically I have a few thoughts about what could make it less annoying for anyone else who stumbles across it:

      • `BsonMemberMap` could have an equality override to compare two member maps (so `==` would work). Probably not a good as to work well you would only want compare the `MethodInfo` (the only commonality between a fresh instance and the existing). Comparing other properties that you can define on `BsonMemberMap` (eg. element name) would make it nearly always fail anyway so it seems kinda pointless.
      • Throw an error with a better description earlier when you detect a mismatch between the member maps available in the class map to what was set via `SetExtraElementsMember`. This could be in the `Freeze` method when you do your lookup to get the member map for extra elements. Alternatively, when you actually call `SetExtraElementsMember`, you could check then if the member is actually mapped and error if it isn't.
      • Update inline documentation for `SetExtraElementsMember` and state that the member map specified needs to be the same instance as one of the member maps on the class map.

            Assignee:
            Unassigned Unassigned
            Reporter:
            james@turnersoftware.com.au James Turner
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Created:
              Updated: