Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static command don't resolve viewmodel correctly #809

Open
martindybal opened this issue Mar 4, 2020 · 7 comments · May be fixed by #1898
Open

Static command don't resolve viewmodel correctly #809

martindybal opened this issue Mar 4, 2020 · 7 comments · May be fixed by #1898
Assignees
Labels
Milestone

Comments

@martindybal
Copy link
Contributor

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

@tomasherceg
Copy link
Member

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.

@martindybal
Copy link
Contributor Author

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

@exyi
Copy link
Member

exyi commented Mar 4, 2020

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
Copy link
Member

exyi commented Mar 29, 2020

@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"?

@tomasherceg
Copy link
Member

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.

@exyi exyi closed this as completed May 14, 2020
@exyi exyi reopened this May 14, 2020
@exyi
Copy link
Member

exyi commented May 14, 2020

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.

@tomasherceg tomasherceg added this to the Version 2.5 milestone May 31, 2020
@tomasherceg tomasherceg modified the milestones: Version 2.5, Version 3.0 Jun 12, 2020
@tomasherceg
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants