dotvvm icon indicating copy to clipboard operation
dotvvm copied to clipboard

Static command don't resolve viewmodel correctly

Open martindybal opened this issue 4 years ago • 7 comments

public class SomeDependency
{
    public string SomeProperty { get; set; }
}

public class MasterViewModel : DotvvmViewModelBase
{
    private string dependencySomeProperty;

    public MasterViewModel(SomeDependency dependency)
    {
        //System.NullReferenceExceptionObject reference not set to an instance of an object.
        dependencySomeProperty = dependency.SomeProperty;
    }
}

public class PageViewModel : MasterViewModel
{
    public PageViewModel(SomeDependency dependency) : base(dependency)
    {
    }

    [AllowStaticCommand]
    public void StaticCommand()
    {
    }
}

Dependencies are null when static command is executed so I get NullReferenceException in constructor.

DotVVM version: 2.4.0.1

martindybal avatar Mar 04 '20 08:03 martindybal

Calling instance methods on viewmodels from static commands is not supported. You should use Static Command Services.

Since the static commands don't send the viewmodel to the server, property values of the viewmodel are unknown and the object will be in a strange and probably inconsistent state. The same applies to the dependencies passed in the constructor - for historical reasons, they are set to null because it was implemented before DotVVM was using IServiceProvider. We could "fix" this by resolving them from the IServiceProvider, but still - the viewmodel will be in a weird state, the lifecycle methods won't be called, and the method may rely on data which are not there. We should throw an exception in this case - I think that the probability of using this feature by mistake is greater than the probability that someone is using it intentionally and is happy with how it behaves.

tomasherceg avatar Mar 04 '20 14:03 tomasherceg

Oh, I see. I think the exception is best solution of this

martindybal avatar Mar 04 '20 15:03 martindybal

dependencies passed in the constructor - for historical reasons, they are set to null because it was implemented before DotVVM was using IServiceProvider.

I don't think so, the arguments of a static command are simply deserialized using Newtonsoft.Json and, apparently it passes nulls to constructors (if a property of the same name does not exist).

This is how arguments are processed: https://github.com/riganti/dotvvm/blob/master/src/DotVVM.Framework/Hosting/DotvvmPresenter.cs#L332

exyi avatar Mar 04 '20 16:03 exyi

@tomasherceg What can we do about this? Shall we also use the dotvvm serializer static command arguments? (this is certainly a breaking change) Or call it a "feature"?

exyi avatar Mar 29 '20 09:03 exyi

I think that the only reason for using an instance method as a target of static commands is the dependency injection, so we can change the behavior and make a breaking change here - I don't think we'll break someone as this entire situation is quite rare, and if the viewmodel instance can be created for the original page load, it should resolve for a static command as well.

tomasherceg avatar May 13 '20 18:05 tomasherceg

Now, I don't know if you mean banning everything except @service in the instance parameter or introducing a different way to deserialize it by not deserializing it at all and only filling in services.

In any case, it does not really solve the problem, since we are still deserializing the other parameters in this way.

exyi avatar May 14 '20 09:05 exyi

We've discussed this and it is a breaking change so it needs to go to v3.

  • Restrict instance method calls only to static command services.
  • Parameters should be deserialized with our deserializer.
  • We need to restrict signed and encrypted values in parameters - there is no way how to validate them, so we need to throw an exception.

tomasherceg avatar Jun 12 '20 12:06 tomasherceg