cloudevents / sdk-csharp

CSharp SDK for CloudEvents
Apache License 2.0
282 stars 82 forks source link

CloudEventAttribute should implement equality operators #229

Open jstafford5380 opened 2 years ago

jstafford5380 commented 2 years ago

CloudEvent.GetPopulatedAttributes() returns an IEnumerable<KeyValuePair<CloudEventAttribute, object>> which is actually a bit odd. You'd think it would be usable as a dictionary, but I'm guessing there might be a cause where the same attribute names appear more than once, but in my case, I know they won't, so I tried to create a dictionary from it, but unfortunately, since the Key is CloudEventAttribute, indexing it requires equality operators. Even if I simply search the list of KVPs, it makes it awkward to find an attribute that may or may not exist (SingleOrDefault for example, because default is a KVP with null values), so I think I have to loop on it the old fashioned way.

I find the interface in general to be very awkward, but I think some comparison operators would be helpful, or even a helper method that can grab the attribute and/or value by name. There's already a method to get the attribute, there's just no value in it; it's only the name. Maybe there's some other incantation I'm not seeing; the documentation on extension attributes is lacking in general.

jskeet commented 2 years ago

I will look at this on Monday when I'm back from holiday. It's relatively awkward to make CloudeventAttribute plenty equality due to constraints - unless we decided to make that not part of the equality.

jstafford5380 commented 2 years ago

Thanks Jon. I wonder if a custom collection object could help. Just spitballing... maybe something that would be used like this:

AttributeCollection col = ce.GetPopulatedAttributeCollection();
IEnumerable<CloudEventAttribute> myAttrs = col.FindByName("myAttr");

It would probably be expected, of course, that the object implements ICollection at the least.

jskeet commented 2 years ago

We could add extension methods for that easily without a new collection type. Anyway, will look more carefully on Monday.

jstafford5380 commented 2 years ago

We could add extension methods for that easily without a new collection type. Anyway, will look more carefully on Monday.

This is where my head first to be honest, so I'm with you. I backpedaled to the custom object because it almost felt like there was potentially room for more function around working with those attributes, but in retrospect, those cases probably aren't related to the collection itself.

jskeet commented 2 years ago

Looking at this again, while introducing some equality comparers might still be useful, I think for your current use case you could just use:

var attributes = ce.GetPopulatedAttributes().ToDictionary(pair => pair.Key.Name, pair => pair.Value);

That would then give you a Dictionary<string, object> which I suspect is what you want, without any change to the current code at all :)

Does that work for you?

jstafford5380 commented 2 years ago

For this use case, yep this works just fine and is actually how I moved on; I'm good to go. This issue is just meant as an enhancement request for the API

jskeet commented 2 years ago

Okay. I'll keep going with the IEqualityComparer PR then. That would allow something like:

Dictionary<CloudEventAttribute, object> dict = new(ce.GetPopulatedAttributes(), CloudEventAttribute.NameComparer);