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: https://github.com/MarimerLLC/csla/blob/579c84dd14398549c1b48be3892d543a1a956260/Source/Csla/Core/FieldManager/FieldData.cs#L28-L42

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: https://github.com/MarimerLLC/csla/blob/0f469350a0d72bf35d35deebb6660c6231c50b5d/Source/Csla/ApplicationContext.cs#L143-L147 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.

[JasonBock]

If a property can be null, it needs to be annotated as such. And yes, that means that every usage of that property needs to do a null check. If the context should never be null, it should not be annotated with ?, but, arguably, when you have a mutable property, you will need to do this regardless. The annotation is essentially a compilation helper, in that it can provide warnings to users to not do bad things with members that don't expect values to be null. But it doesn't stop it. You still have to check for null. And when the property is mutable, it means you always have to check it.

Consider this:

public sealed class Data
{
  public Data(string value)
  {
    ArgumentNullException.ThrowIfNull(value);
    this.Value = value;
  }

  public string Value { get; }
}

Since the property isn't settable after construction, there's no way it could be null. If it was, you would've received an exception during the creation of a Data instance. (Yes, you could do Reflection shenanigans to change that property value via the backing field, but I personally wouldn't be concerned with people digging themselves into holes this route). Therefore, I don't have to worry about checking Value all the time for a null value.

But if I do this:

public sealed class Data
{
  public Data(string value)
  {
    ArgumentNullException.ThrowIfNull(value);
    this.Value = value;
  }

  public string Value { get; set; }
}

Now I have to check for null every time I access Value. To keep the semantics of "Value will never be null", I would need to change the setter to do a null check, which would then ensure that Value wouldn't be null.

Relating this back to ClientContext, either you annotate it with ? because it's possible it could be null, or you don't, and then ensure that ClientContext can never be null. As you mentioned, this may mean you to provide an "empty" or "default" ClientContext instance. I don't know how that would work in CSLA. Or, since the current behavior is that it could be null, then put ? on it, and make it explicit for callers.

[StefanOssendorf]

Of course if a 3rd party can directly modifiy a property we have to make that nullable or change it to a backing field with a guard clause. In the case of ClientContext we have full control over it due to get/set methods where we can deny null as an acceptable value. The question is should we do that or not? I personally would say: Yes, we should. It would not only make our code more robust and clean but our users one, too.

[JasonBock]

The current behavior states that null is a valid value, so it should be annotated with ?. However, I'm guessing this will be part of a breaking change release, so maybe the right thing to do is to not annotate it, and force that either a non-null reference is provided, or an exception will occur. We know what will happen if we leave it nullable, because that's the current state of CSLA. Making it non-nullable may be the "right" change. I'm guessing @rockfordlhotka should weigh in on this.

[StefanOssendorf]

@JasonBock Yes. I'm asking because we are working on a breaking change version. Otherwise you are perfectly right. I would have annotatd it and keep the null behavior. Since I'm adding it and we are in a breaking change version I'd like to make it a bit more nice imho :-)

[StefanOssendorf]

Thinking about it. The nullable feature would mean you can't use any auto properties because they don't adhere to the annotations. There are cases even in the asp.net core framework like HttpContext.Items which are not nullable but the framework does not prevent you from setting it null. So my stand will probably be: We use the feature so respect the feature. Otherwise garbage in -> garbage out :D

[rockfordlhotka]

The data portal has optimizations to minimize data on the wire through the use of null.

So if someone never uses a collection like ClientContext, then only a null token is serialized instead of the empty collection, which is much larger because it includes the type info.

As a result, anyone using a remote data portal is helped anytime a complex type has a null value instead of an instance of the type.

[rockfordlhotka]

When it comes to ClientContext (and LocalContext) the current implementation is to ensure that no one ever gets a null value, but behind the scenes the value might be null so we get the serialization optimization.

This means that the getter is what ensures that the value is never null. If the value is null, the setter creates it and returns the value.

That way, if someone doesn't use those context values (and this is a lot of users), then they pay no price for the feature existing in CSLA. But if someone does use the context dictionaries, then they are created on demand.

As a result, in my view, the public API shouldn't change - people shouldn't worry that the hidden/private value behind the scenes could be null. We can change the private implementation if desired - annotating the field with ?, and the getter keeps doing what it does today.

I don't see any good reason to break everyone's code with a public API change. Either they use the context dictionaries and the dictionaries get lazy created, or they don't use them and the private field behind the scenes remains null.

[StefanOssendorf]

The data portal has optimizations to minimize data on the wire through the use of null.

So if someone never uses a collection like ClientContext, then only a null token is serialized instead of the empty collection, which is much larger because it includes the type info.

As a result, anyone using a remote data portal is helped anytime a complex type has a null value instead of an instance of the type.

That's resonable. For me the correct place for such optimization would be the serializer and not the class to be serialized. But that's out of scope and maybe we'll tackle it at a later date.

[StefanOssendorf]

@rockfordlhotka Could you please take a look at Csla.DataPortalClient.DataPortalProxyDescriptor. This class is no where referenced. Neither by type or as a magic string. Otherwise we can remove it. And Csla.DataPortalClient.IApplicationContextProvider looks dead, too.

[StefanOssendorf]

Please also take a look at Csla.DataPortalClient.LocalProxyExtensions.UseLocalProxy(config, options). That implementation looks broken to me 🤔

[StefanOssendorf]

Please look at Csla.Reflect.DynamicMethodHandle ctor. The parameter parameters isn't used. Either that's a bug or it can be removed. For the latter I'll remove it :)

[rockfordlhotka]

DynamicMethodHandle

It appears in use to me?

        if (parameters == null)
        {
          inParams = [null];

        }
        else
        {
          inParams = parameters;
        }

[StefanOssendorf]

DynamicMethodHandle

It appears in use to me?

        if (parameters == null)
        {
          inParams = [null];

        }
        else
        {
          inParams = parameters;
        }

Yes. But inParams is not used. So when I remove inParams parameters is now unused, too.

[rockfordlhotka]

I think we need to create an issue to actually use the parameters. They are passed in when the ctor is called, and presumably are supposed to be used to ensure we use the right overload of the method.

[StefanOssendorf]

I'm now working on Csla.Rules.

[StefanOssendorf]

@rockfordlhotka I need some input please :) We have these two rules: MinValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L371-L377 and MaxValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L459-L465

What would you expect to happen, when value == null in the line of comparison? That's a possibility there. We can decide it counts as not violating the rule or we make the rule restriction tighter. For example T must be a struct.

[StefanOssendorf]

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... https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98 https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

[rockfordlhotka]

@rockfordlhotka I need some input please :) We have these two rules: MinValue<T> where T: IComparable

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L371-L377

and MaxValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L459-L465

What would you expect to happen, when value == null in the line of comparison? That's a possibility there. We can decide it counts as not violating the rule or we make the rule restriction tighter. For example T must be a struct.

I think in this context we should consider null to be default(T) for the purpose of comparison.

[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...

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

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.

[StefanOssendorf]

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... https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

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.

I understand that. But List<> isn't serializable and a rule context doesn't travel between client and server. So I think this should be okay here.

[StefanOssendorf]

@rockfordlhotka I need some input please :) We have these two rules: MinValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L371-L377

and MaxValue<T> where T: IComparable https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/CommonRules.cs#L459-L465

What would you expect to happen, when value == null in the line of comparison? That's a possibility there. We can decide it counts as not violating the rule or we make the rule restriction tighter. For example T must be a struct.

I think in this context we should consider null to be default(T) for the purpose of comparison.

I don't understand your answer. My problem ist that value can be null which means I can't do value.CompareTo() because of a NullReferenceException. How do we handle that?

[rockfordlhotka]

I don't understand your answer. My problem ist that value can be null which means I can't do value.CompareTo() because of a NullReferenceException. How do we handle that?

The intent of the code is that if the value is null that it would be replaced with default(T), and so handles a lot of common scenarios.

If default(T) is null, I'm not sure exactly what to do. As an undefined operation, we could assume true or false - maybe use a random number generator? 😎

Realistically, we should probably add an error result indicating that "value == null"

We should probably special-case string to be string.Empty - that'd be a common scenario where the rule should (imo) work.

[StefanOssendorf]

Please take a look at Csla.Security.PrincipalCache this looks dead, too. If it's dead we also can remove the SecurityOptions.PrincipalCacheMaxCacheSize property.

[StefanOssendorf]

ICslaIdentity, ICslaPrincipal, ICheckRoles. also dead? 🤔

[StefanOssendorf]

Next is Csla.Server

[StefanOssendorf]

@rockfordlhotka

• Csla.DataPortalClient.DataPortalProxy.GetBaseCriteriaRequest()
• Csla.DataPortalClient.DataPortalProxy.GetBaseUpdateCriteriaRequest()

Both set the ClientCulture and ClientUICulture property of the request object. But in different ways. System.Globalization.CultureInfo.CurrentCulture.Name vs Thread.CurrentThread.CurrentCulture.Name.

Is that intended?

[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]

https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/Server/ObjectFactory.cs#L62-L75

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

[rockfordlhotka]

https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/Server/ObjectFactory.cs#L62-L75

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]

https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/Server/ObjectFactory.cs#L62-L75

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]

https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/Server/ObjectFactory.cs#L62-L75

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... https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

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... https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L84-L98

https://github.com/MarimerLLC/csla/blob/aa14c3f86e3052d73cb06785b6fb6c381e6eb9f2/Source/Csla/Rules/RuleContext.cs#L66-L82

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 https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/DataPortalT.cs#L119-L136

[rockfordlhotka]

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

https://github.com/MarimerLLC/csla/blob/e035ad787444a8a3f6b5df603d472030c124efc6/Source/Csla/DataPortalT.cs#L119-L136

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.