I keep forgetting Base objects can't use "Item" as a dynamic member

jonathon · 15 July 2022 18:17

When The Rant Over GIF - Rant Rant Over End Of Rant - Discover & Share GIFs

AlanRynne · 15 July 2022 18:20

LOL, I found this on our code this week, so your guess is as good as mine at this point

specklesystems/speckle-sharp/blob/f97af0dbff3a1982d0f5567a0bab0fb050da18ed/Core/Core/Models/DynamicBase.cs#L152

      
        
            /// </summary>
            /// <returns></returns>
            public override IEnumerable<string> GetDynamicMemberNames()
            {
              var names = new List<string>();
              foreach (var kvp in properties) names.Add(kvp.Key);
            
            
  var pinfos = GetType().GetProperties(BindingFlags.Instance | BindingFlags.Public);
              foreach (var pinfo in pinfos) names.Add(pinfo.Name);
            
            
  names.Remove("Item"); // TODO: investigate why we get Item out?
              return names;
            }
            
            
/// <summary>
            /// Gets the names of the defined class properties (typed).
            /// </summary>
            /// <returns></returns>
            public IEnumerable<string> GetInstanceMembersNames()
            {
              var names = new List<string>();

jonathon · 15 July 2022 18:23

Item is a member of IDictionary but as Item[]

So if you add Item[] as a member that is totally fine.

The exception doesn’t give many clues: Parameter Count Mismatch.

BaseObject["Item"] => error
BaseObject["Item[]"] => fine

I’ve added a sanitization routine for property names now - perhaps that should be in Core.Core.Models.DynamicBase itself under IsPropertyNameValid

AlanRynne · 15 July 2022 18:56

I’ll let @dimitrie weigh in on this one, but I’m not against it

dimitrie · 19 July 2022 05:49

It’s still kicking the can down the road. Item is problematic because of DynamicObject, IDynamicMetaObjectProvider inheritance of Base. I’d happily be in favour of swapping that for a simpler Base : Dictionary<string, object> - which is perfectly ok functionality wise, though a drop-in replacement might be difficult…

dimitrie · 20 July 2022 07:57

Okay, I’ve dug into this a bit more. Item was actually hidden in plain sight, as this property:

github.com

specklesystems/speckle-sharp/blob/d2cfe0fdf5dac5e5b1ae06f2f4a063cf33dc15ff/Core/Core/Models/DynamicBase.cs#L98-L138

      
        
            public object this[string key]
            {
              get
              {
                if (properties.ContainsKey(key))
                  return properties[key];
            
            
    var prop = GetType().GetProperty(key);
            
            
    if (prop == null)
                  return null;
            
            
    return prop.GetValue(this);
              }
              set
              {
                if (!IsPropNameValid(key, out string reason)) throw new SpeckleException("Invalid prop name: " + reason);
            
            
    if (properties.ContainsKey(key))
                {

This file has been truncated. show original

I have a local fix that allows for Items to be set as dynamic props, as well as typed properties inside custom classes (also doing some fly-by caching and DRY-ing things up). Now I’ve also got the itch to cleanup the mess that accumulated in there re getting members from objects - I’ll leave that for another holiday though.

PR here: fix(core): allows for `Item` to be set as a dynamic or typed prop in `Base`-derived objects by didimitrie · Pull Request #1422 · specklesystems/speckle-sharp · GitHub

jonathon · 20 July 2022 13:35

Great. It was only a rant that I had previously stumbled on, worked around, and then forgot about when encountering it again.

Property names added by the dictionary method skip the validity check for serialization. I’ve raised a PR to the docs with a note about that.