FoundatioFx / Foundatio

Pluggable foundation blocks for building distributed apps.
Apache License 2.0
1.99k stars 244 forks source link

DataDictionary.Empty is not protected by potential bugs. #280

Open frabe1579 opened 2 years ago

frabe1579 commented 2 years ago

I've found a potential source of bugs: the class Foundatio.Utility.DataDictionary exposes a static readonly field Empty to represent empty dictionaries, but it's instance is not readonly, so values can be added to Empty causing potential bugs. The current declaration is:

public static readonly DataDictionary Empty = new();

A new safe declaration shoud be

public static DataDictionary Empty => new();

This at least returns always empty dictionaries, but creates a new instance at every call. Another approach should be to re-implement DataDictionary with explicit implementation of IDictionary and IReadOnlyDictionary, or something else.

niemyjski commented 2 years ago

Yes, we should work on making empty be read-only. @ejsmith thoughts on above, kind of would be nice to make it truly readonly instead of returning a new instance. But would take a little bit of work.

ejsmith commented 2 years ago

Yeah, this is an issue. We would definitely want to return an empty read only instance and it should be the same instance for all usages to avoid allocating a dictionary.

frabe1579 commented 2 years ago

Maybe a IReadOnlyDictionary<,>, with appropriate extension methods, should be used when exposed for example on IQueueEntry<>, but I don't know if it's used in other code.