Use nullable reference types throughout CSLA codebase

[JasonBock]

Is your feature request related to a problem? Please describe.
C#8 adds nullable reference types. Not only will CSLA need to support this, but it should also annotate its members with not just ?, but with the new nullable attributes.

Describe the solution you'd like
NRT support throughout CSLA.

Describe alternatives you've considered
Nothing :)

Additional context
Use this reference for details/information.

[StefanOssendorf]

@rockfordlhotka
I'd love to tackle this issue. I don't like null :-)
Btw the nullable annotation context is not a breaking change. The annotation is only a compiler feature during development and not something the runtime checks.

[rockfordlhotka]

I'd love to have you do this!

It'll be CSLA 9 thing, as I'm sure there'll be breaking changes.

[StefanOssendorf]

@rockfordlhotka I'll link you here from now on for questions regarding the nullable context :-)
First question:

csla/Source/Csla/Core/FieldManager/FieldData.cs

Lines 28 to 42 in 579c84d

	/// <summary>
	/// Creates a new instance of the object.
	/// </summary>
	public FieldData() { }
	/// <summary>
	/// Creates a new instance of the object.
	/// </summary>
	/// <param name="name">
	/// Name of the field.
	/// </param>
	public FieldData(string name)
	{
	Name = name;
	}

Is the empty ctor only there for the MobileFormatter? It's at least not used within CSLA itself. I'd love to mark it Obsolete(true) because it would remove a lot of "maybe nullable" things in the class itself. For example the name can than be guaranteed.

[rockfordlhotka]

MobileFormatter does require an empty default ctor. Whether that is explicit or implicit, it is required for all types that implement IMobileObject.

[StefanOssendorf]

In that case I'll mark it as Obsolete. MobileFormatter can still use it but it's not usable any more at compile time :-)

[StefanOssendorf]

And should I really add now everywhere a null argument check where it's currently just assumed to be not null? 😅🤔

[rockfordlhotka]

That seems wise doesn't it?

[JasonBock]

I concur with @rockfordlhotka. If a parameter is a NRT, someone could still pass in null, so an ArgumentNullException.ThrowIfNull() call is warranted.

[rockfordlhotka]

Hey - this is now part of the permanent record!! @JasonBock agrees with me!! 😁

[JasonBock]

@rockfordlhotka don't make me do it....

[StefanOssendorf]

@rockfordlhotka How should we handle null within the framework to e.g. reset/unset the ClientContext?
How do we handle things like this:

csla/Source/Csla/ApplicationContext.cs

Lines 143 to 147 in 0f46935

	public void Clear()
	{
	SetContext(null);
	ContextManager.SetLocalContext(null);
	}

I personally don't like to set something null when I just can assign a new instance to avoid that. At the cost of memory consumption obviously.
But - and I think that's the biggest downside on allwoing null in such cases - we have to mark ClientContext as nullable which means every code says "Hey! This could be null here. Please check that" eventhough we know that won't ever be the case. But that's not expressable by the current annotations as far as I am aware.

[rockfordlhotka]

@StefanOssendorf that is interesting. I suspect one was written (or updated) after the other. I think the current best approach is to use the Globalization namespace, but I'm not sure.

[StefanOssendorf]

Is the type Csla.Server.MobileFactoryAttribute obsolete? The documentation is talking about WCF and it's not used within the sources.

[StefanOssendorf]

csla/Source/Csla/Server/ObjectFactory.cs

Lines 62 to 75 in e035ad7

	/// <summary>
	/// Calls the ValidationRules.CheckRules() method
	/// on the specified object, if possible.
	/// </summary>
	/// <param name="obj">
	/// Object on which to call the method.
	/// </param>
	protected async Task CheckRulesAsync(object obj)
	{
	if (obj is IDataPortalTarget target)
	await target.CheckRulesAsync().ConfigureAwait(false);
	else
	MethodCaller.CallMethodIfImplemented(obj, "CheckRules", null);
	}

Is it a bug that the MethodCaller.CallMethodIfImplemented part isn't trying to invoke the async version of check rules?

[rockfordlhotka]

csla/Source/Csla/Server/ObjectFactory.cs

Lines 62 to 75 in e035ad7

	/// <summary>
	/// Calls the ValidationRules.CheckRules() method
	/// on the specified object, if possible.
	/// </summary>
	/// <param name="obj">
	/// Object on which to call the method.
	/// </param>
	protected async Task CheckRulesAsync(object obj)
	{
	if (obj is IDataPortalTarget target)
	await target.CheckRulesAsync().ConfigureAwait(false);
	else
	MethodCaller.CallMethodIfImplemented(obj, "CheckRules", null);
	}

Is it a bug that the MethodCaller.CallMethodIfImplemented part isn't trying to invoke the async version of check rules?

Yes. That whole thing exists as a fallback for an edge case I suspect no one actually uses, but for completeness it should be made to work.

[StefanOssendorf]

csla/Source/Csla/Server/ObjectFactory.cs

Lines 62 to 75 in e035ad7

	/// <summary>
	/// Calls the ValidationRules.CheckRules() method
	/// on the specified object, if possible.
	/// </summary>
	/// <param name="obj">
	/// Object on which to call the method.
	/// </param>
	protected async Task CheckRulesAsync(object obj)
	{
	if (obj is IDataPortalTarget target)
	await target.CheckRulesAsync().ConfigureAwait(false);
	else
	MethodCaller.CallMethodIfImplemented(obj, "CheckRules", null);
	}

Is it a bug that the MethodCaller.CallMethodIfImplemented part isn't trying to invoke the async version of check rules?

Yes. That whole thing exists as a fallback for an edge case I suspect no one actually uses, but for completeness it should be made to work.

Yeah. But shouldn't the fallback also tries to invoke CheckRulesAsync instead of just CheckRules?

[rockfordlhotka]

MobileFactoryAttribute

Yes. I'm not actually sure what that was for or where it came from. Looks like a duplicate of ObjectFactoryAttribute in some ways.

[rockfordlhotka]

csla/Source/Csla/Server/ObjectFactory.cs

Lines 62 to 75 in e035ad7

	/// <summary>
	/// Calls the ValidationRules.CheckRules() method
	/// on the specified object, if possible.
	/// </summary>
	/// <param name="obj">
	/// Object on which to call the method.
	/// </param>
	protected async Task CheckRulesAsync(object obj)
	{
	if (obj is IDataPortalTarget target)
	await target.CheckRulesAsync().ConfigureAwait(false);
	else
	MethodCaller.CallMethodIfImplemented(obj, "CheckRules", null);
	}

Is it a bug that the MethodCaller.CallMethodIfImplemented part isn't trying to invoke the async version of check rules?

Yes. That whole thing exists as a fallback for an edge case I suspect no one actually uses, but for completeness it should be made to work.

Yeah. But shouldn't the fallback also tries to invoke CheckRulesAsync instead of just CheckRules?

Yes, that is what I'm saying. It should be made to work - which means trying to invoke both.

[JasonBock]

Another question: Any good reason why this lists can't just be initialized and hence empty if not neede and/or used? Makes the nullable stuff a lot easier and even more intuitive to use. Currently accessing DirtyProperties in a rule would return null instead of an empty list...

csla/Source/Csla/Rules/RuleContext.cs

Lines 84 to 98 in aa14c3f

	private readonly LazySingleton<Dictionary<IPropertyInfo, object>> _outputPropertyValues;
	/// <summary>
	/// Gets a dictionary containing copies of property values that
	/// should be updated in the target object.
	/// </summary>
	[EditorBrowsable(EditorBrowsableState.Never)]
	public Dictionary<IPropertyInfo, object> OutputPropertyValues
	{
	get
	{
	if (!_outputPropertyValues.IsValueCreated)
	return null;
	return _outputPropertyValues.Value;
	}
	}

csla/Source/Csla/Rules/RuleContext.cs

Lines 66 to 82 in aa14c3f

	private LazySingleton<List<IPropertyInfo>> _dirtyProperties;
	/// <summary>
	/// Gets a list of dirty properties (value was updated).
	/// </summary>
	/// <value>
	/// The dirty properties.
	/// </value>
	[EditorBrowsable(EditorBrowsableState.Never)]
	public List<IPropertyInfo> DirtyProperties
	{
	get
	{
	if (!_dirtyProperties.IsValueCreated)
	return null;
	return _dirtyProperties.Value;
	}
	}

CSLA often doesn't initialize lists of types that might be serialized, because it is much smaller to serialize null than it is to serialize an empty list. This is because the serializer doesn't need to flow the list's type information for null, but it does need to flow the type information for an empty list so the empty list can be deserialized on the other end of the process.

For MobileFormatter, yes, you need to serialize type information for a collection type so you know what to create on the other side. But that isn't a strict requirement for CSLA serialization. For example, my source generator serializer doesn't require this, because it knows at compile-time what the underlying type is for the field, and can new it up how it sees fit. For collections like List<T>, I don't special-case that one, but it's trivial to add a custom implementation for that in my generator through configuration.

Also, I do like empty collections over null in general overall. Then I don't need to wonder, "is this a null list or not?" everywhere in my code. If the performance implications of a null list over an empty one is significant during serialization, then....maybe keep them null, but if it's essentially negligible, then I'd much rather see an empty list.

[rockfordlhotka]

Another question: Any good reason why this lists can't just be initialized and hence empty if not neede and/or used? Makes the nullable stuff a lot easier and even more intuitive to use. Currently accessing DirtyProperties in a rule would return null instead of an empty list...

csla/Source/Csla/Rules/RuleContext.cs

Lines 84 to 98 in aa14c3f

	private readonly LazySingleton<Dictionary<IPropertyInfo, object>> _outputPropertyValues;
	/// <summary>
	/// Gets a dictionary containing copies of property values that
	/// should be updated in the target object.
	/// </summary>
	[EditorBrowsable(EditorBrowsableState.Never)]
	public Dictionary<IPropertyInfo, object> OutputPropertyValues
	{
	get
	{
	if (!_outputPropertyValues.IsValueCreated)
	return null;
	return _outputPropertyValues.Value;
	}
	}

csla/Source/Csla/Rules/RuleContext.cs

Lines 66 to 82 in aa14c3f

	private LazySingleton<List<IPropertyInfo>> _dirtyProperties;
	/// <summary>
	/// Gets a list of dirty properties (value was updated).
	/// </summary>
	/// <value>
	/// The dirty properties.
	/// </value>
	[EditorBrowsable(EditorBrowsableState.Never)]
	public List<IPropertyInfo> DirtyProperties
	{
	get
	{
	if (!_dirtyProperties.IsValueCreated)
	return null;
	return _dirtyProperties.Value;
	}
	}

CSLA often doesn't initialize lists of types that might be serialized, because it is much smaller to serialize null than it is to serialize an empty list. This is because the serializer doesn't need to flow the list's type information for null, but it does need to flow the type information for an empty list so the empty list can be deserialized on the other end of the process.

For MobileFormatter, yes, you need to serialize type information for a collection type so you know what to create on the other side. But that isn't a strict requirement for CSLA serialization. For example, my source generator serializer doesn't require this, because it knows at compile-time what the underlying type is for the field, and can new it up how it sees fit. For collections like List<T>, I don't special-case that one, but it's trivial to add a custom implementation for that in my generator through configuration.

Also, I do like empty collections over null in general overall. Then I don't need to wonder, "is this a null list or not?" everywhere in my code. If the performance implications of a null list over an empty one is significant during serialization, then....maybe keep them null, but if it's essentially negligible, then I'd much rather see an empty list.

I split this into its own issue (#4266) so we can discuss better.

[StefanOssendorf]

Is it intended that an aggregate exception populates an exception but a DataPortalException resumes the execution flow? 🤔
Line 131 vs 135

csla/Source/Csla/DataPortalT.cs

Lines 119 to 136 in e035ad7

	try
	{
	result = await Cache.GetDataPortalResultAsync(objectType, criteria, DataPortalOperations.Create,
	async () => await proxy.Create(objectType, criteria, dpContext, isSync));
	}
	catch (AggregateException ex)
	{
	if (ex.InnerExceptions.Count > 0)
	{
	if (ex.InnerExceptions[0] is Server.DataPortalException dpe)
	HandleCreateDataPortalException(dpe);
	}
	throw new DataPortalException($"DataPortal.Create {Resources.Failed}", ex, null);
	}
	catch (Server.DataPortalException ex)
	{
	HandleCreateDataPortalException(ex);
	}

[rockfordlhotka]

Is it intended that an aggregate exception populates an exception but a DataPortalException resumes the execution flow? 🤔 Line 131 vs 135

csla/Source/Csla/DataPortalT.cs

Lines 119 to 136 in e035ad7

	try
	{
	result = await Cache.GetDataPortalResultAsync(objectType, criteria, DataPortalOperations.Create,
	async () => await proxy.Create(objectType, criteria, dpContext, isSync));
	}
	catch (AggregateException ex)
	{
	if (ex.InnerExceptions.Count > 0)
	{
	if (ex.InnerExceptions[0] is Server.DataPortalException dpe)
	HandleCreateDataPortalException(dpe);
	}
	throw new DataPortalException($"DataPortal.Create {Resources.Failed}", ex, null);
	}
	catch (Server.DataPortalException ex)
	{
	HandleCreateDataPortalException(ex);
	}

I am pretty sure this is intended.

If a DataPortalException is what occurs, that can safely flow up through the call stack. If an AggregateException occurs then that needs to be converted into a DataPortalException that can safely flow up through the call stack.

[StefanOssendorf]

But that's my question. A DataPortalException isn't bubbling up to the caller but catched and "handled". Where as the AggregateException packs it into a DataPortalException and throws that.

[rockfordlhotka]

HandleCreateDataPortalException

I think the code is correct as-is. The HandleDataPortalException method exists to unpack and remove any top-level server-side DataPortalException before throwing the client-side DataPortalException. In the case of an AggregateException being caught directly on the client, if it isn't a server DataPortalException, then the exception can be directly wrapped in a client DataPortalException as it is thrown.

[StefanOssendorf]

Yep. You are totally right. I thought the Handle* method is an extension point for users and resumes the execution flow afterwards. But it throws an exception 🤦🏻‍♂️

[StefanOssendorf]

Another look please :-)

csla/Source/Csla/DynamicListBase.cs

Lines 266 to 279 in 6789bb5

	public async void SaveItem(int index)
	{
	try
	{
	await SaveItemAsync(index);
	}
	catch (AggregateException ex)
	{
	if (ex.InnerExceptions.Count > 0)
	throw ex.InnerExceptions[0];
	else
	throw;
	}
	}

IMHO this is bad and a bug. I don't know if this base class is really in use.
async void for non event handler should be banned ^^'. If I read this right this will not bubble up any exceptions to the user during a save.

Edit:
Here too:

csla/Source/Csla/DynamicListBase.cs

Line 346 in 6789bb5

protected override async void RemoveItem(int index)

[rockfordlhotka]

Another look please :-)

csla/Source/Csla/DynamicListBase.cs

Lines 266 to 279 in 6789bb5

	public async void SaveItem(int index)
	{
	try
	{
	await SaveItemAsync(index);
	}
	catch (AggregateException ex)
	{
	if (ex.InnerExceptions.Count > 0)
	throw ex.InnerExceptions[0];
	else
	throw;
	}
	}

IMHO this is bad and a bug. I don't know if this base class is really in use.
async void for non event handler should be banned ^^'. If I read this right this will not bubble up any exceptions to the user during a save.
Edit: Here too:

csla/Source/Csla/DynamicListBase.cs

Line 346 in 6789bb5

protected override async void RemoveItem(int index)

I don't disagree in principal, but iirc this is required by Windows Forms data binding so I don't know that it can be fixed.

Hmm, maybe it can - is there also a DynamicBindingListBase? Yes there is. So we can probably break the API for DynamicListBase because it isn't limited to that particular data binding behavior.

[StefanOssendorf]

Hey everyone, I need some opinions on how to tackle NRTs :)

I've added NRT to the Csla project and am now reiterating it get rid of warnings produced during the change.

I have (currently) two major problems/concerns.

A) Annotating our generic mobile base classes.
As example MobileDictionary:

The declaration of that type must be MobileDictionary<K, V> : Dictionary<K, V> where K: notnull (I'm omitting everything that's not relevant for the NRT stuff)
The SetChildren method uses SerializationInfo as a parameter. And that parameter provides a method T? GetValue<T>(string name).

And here's the problem:
GetValue<T> has to define the return type as T? because the current implementation (everything except relevant code pieces removed, even try-catch to keep it shorter)

public T? GetValue<T>(string name)
{
  var value = _values[name].Value;
  return value != null ? Utilities.CoerceValue<T?>(value.GetType(), null, value) : (T?)value;
}

handles null explicitly so the compiler says: "You are possibly returning null so annotate the return type appropiately".

And now I'm stuck with V (from MobileDictionary) and T? to not match. Because V can be anything (nullable and non-nullable) but T? specifys it could be null even when T may not be nullable at all.

I currently see these options to "solve" that problem:

1. Just add ! to every GetValue invocation and pray the data are correct. (Worst case MobileDictionary<int, string> and we add a null string which is not expected at all)
2. We have to adjust the Type annotations the whole call-chain down and replace a lot of "return null;" cases with "throw InvalidOperationException" to blow up and prevent wrong data
   Maybe you have a better idea?

B)
The same holds for P? GetProperty<P>() which is maybe not expected. (Even more if we add the required property feature)

protected P GetRequiredProperty<P>(PropertyInfo<P> propertyInfo)
{
  if (propertyInfo is null)
  {
    throw new ArgumentNullException(nameof(propertyInfo));
  }
  if (propertyInfo.RelationshipType.HasFlag(RelationshipTypes.LazyLoad))
  {
    throw new NotSupportedException("Lazy load is not allowed by an required property call. Use the normal GetProperty method.");
  }
  ThrowIfNotAllowedToRead();

  return ReadRequiredProperty<P>(propertyInfo);


  // Dummy to show the intention
  static void ThrowIfNotAllowedToRead()
  {
    throw new NotSupportedException();
  }
}

protected P ReadRequiredProperty<P>(IPropertyInfo propertyInfo)
{
  // Throw if property can not be read
  // return property value
}

I'm thinking about adding methods like that where I do not handle any null/default of a type explicitly and just throw an exception if I can't handle it. That would reduce a lot of ! for us and our users.

An example would be:

public static readonly PropertyInfo<string> MyProperty = ...

public string My {
  get => GetProperty<string>(MyProperty)!; // <-- !-operator to suppress the (wrong) null annotation
  set => SetProperty<string>(MyProperty, value);
}

I hope this makes any sense to you guys at all. If not please just ask and I try to clarify as good as I can.

[kant2002]

The question, really - is possibility of null in this place avoidable, because of other reasons, that way slap !, or it's possible in some rare cases.

There is third option - you also can silence warnings using

public T GetValue<T>(string name)
{
  var value = _values[name].Value;
  Debug.Assert(value != null);
  return Utilities.CoerceValue<T?>(value.GetType(), null, value);
}

that way you preserve existing runtime behaviour where you don't have runtime penalty on checking for null and throw NRE in rare cases but in debug builds you can catch that. Also compiler would be happy

You maybe can utilize [return: NotNullIfNotNull("value")] on

csla/Source/Csla/Utilities.cs

Line 153 in a1cebe0

public static object CoerceValue(Type desiredType, Type valueType, object oldValue, object value)

More about it here https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/attributes/nullable-analysis#postconditions-maybenull-and-notnull

[StefanOssendorf]

The null is most of the time introduced because we use return null; or return default;. So it's avoidable but I think only in a disruptive way. Currently we "just" return null but I would like to throw an appropiate exeption telling them what/why it's not going to work anymore and what they have to do to fix it 😅

[StefanOssendorf]

@rockfordlhotka
Is there a reason why SessionMessage derives from CommandBase<T> instead of just being a MobileObject?