-
Notifications
You must be signed in to change notification settings - Fork 97
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
Implemented extensible way to deal with results of commands. This is … #916
Conversation
…crutial use cases when you need to return additional data from static command. One such case is validation results. Which were up until now unpractical to use with static commands. As stattic commands are used more and more prominently due to their performance advantages. Lack features like validation is more and more painful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me overall.
I've updated the code so customData
and result
/ commandResult
would work for classic postback too.
- Please add UI test for the SimpleExceptionFilter test
- Please rename "StaticCommandResult" folder in tests to just "CommandResult" so it better expresses what the sample is for
} | ||
catch (Exception ex) | ||
{ | ||
throw new Exception($"Could not serialize viewModel of type { context.ViewModel.GetType().Name }. Serialization failed at property { writer.Path }. {GeneralViewModelRecommendations}", ex); | ||
throw new Exception($"Could not serialize static command {description} of type '{ data.GetType().FullName}'. Serialization failed at property { writer.Path }. {GeneralViewModelRecommendations}", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's imho missing an article. "Could not serialize a static command {description} object of type ..." sounds a bit better to me.
- Changed interface for CustomData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, how do I access customData on the first (GET) request?
@@ -111,5 +111,7 @@ public interface IDotvvmRequestContext | |||
string? ResultIdFragment { get; set; } | |||
|
|||
IServiceProvider Services { get; } | |||
object? CommandResult { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need this property. Wouldn't it be simpler to just pass the commandResult as an argument to the filter. This way, it's yet another stateful property in IDotvvmrequestContext which may cause confusion when it contains what and when it can be overwritten.
For example, I could write a command:
public void MyCommand() {
Context.CommandResult = new SomeObject();
}
This will return null, which seems wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree that this property does not have to be there. But do you have some idea how to provide an interface to modify the result of command or static command in OnCommandExecutedAsync and etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just add an (ref
) parameter to the OnCommandExecutedAsync
method. Since most people use the base class, we can add a second overload and prevent a breaking change.
@@ -111,5 +111,7 @@ public interface IDotvvmRequestContext | |||
string? ResultIdFragment { get; set; } | |||
|
|||
IServiceProvider Services { get; } | |||
object? CommandResult { get; set; } | |||
Dictionary<string, object>? CustomData { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this to be a dictionary? I think the data should not be readable server-side.
We could just have a method AddCustomData(string key, object data)
. The data will be accessible only client-side. The benefit is that order of execution will not matter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you will need to modify some object in CustomData, the AddCustomData method does not provide you the interface to do so.
You can add some additional CustomData in static command resolution and you can also want to modify it in the OnPresenterExecutedAsync method. This is actually based on one of my use-cases.
For example, you have custom validation and you have multiple places where you just adding errors. At the end of execution, you want to filter the collection for duplicities. If there is just the add method, you cannot filter it.
Also, CustomData was inspired by ModelState (MVC). There is used the same approach as here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am fine with CustomData as a Dictionary (or IDictionary) - HttpContext.Items and other similar containers are also dictionaries. Being able to see what's in the collection can be useful.
But I agree with removing CommandResult from context. Also, I think that allowing the users to change it (in action filters) is a bad idea at all - if the users want to send something else than the result, they can use CustomData and deal with them in the postback handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, unlike ModelState and Items, this is not intended for component-componet communication.
@quigamdev I'd suggest you to rather create a request scoped service that will register your error, then do deduplication and put it into custom data. Then you'll be able to detect cases when you add an error after the deduplication.
When you will need to modify some object in CustomData, the AddCustomData method does not provide you the interface to do so.
That's the purpose - then the order of execution can not matter. In DotVVM we generally have a big problem that things only work at certain time and this will be another source of such issues.
@@ -68,7 +68,7 @@ export async function postBack( | |||
serverResponseObject, | |||
wasInterrupted, | |||
commandResult: null, | |||
response: (err.reason as any).response, | |||
response: (err.reason as any)?.response, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason should not be null, why is the ?.
needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an exception is thrown the reason si null. That's why null propagation is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exception on the server? That would be type: "serverError"
. Exception on the client should also contain the reason when it's of type DotvvmPostbackError
. DotvvmPostbackError
's type definition does not allow null, so could either update it or (preferably IMHO) fix the bug that's causing null in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null in reason
was caused by an exception in dotvvm.events.<>.subscirbe((e)=>{ "some error here" })
. In this case, you need to expect that it can be null. It is non-dotvvm code that will result in this exception and dotvvm has to work with it.
The type definition works only for typescript. But not everyone has to be using it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I'd better add a null check to the constructor. If you terribly need to be null-tollerant, I'd suggest changing the type signature so we can at least be consistent. Even with your additional null checks, the handler is going to crash in isInterruptingErrorReason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@quigamdev I think we should add try/catch blocks around errors in event handlers and wrap the error with reason: event
. But I think we can do it in a separate PR.
src/DotVVM.Framework/Resources/Scripts/postback/postbackCore.ts
Outdated
Show resolved
Hide resolved
} | ||
catch (Exception ex) | ||
{ | ||
throw new Exception($"Could not serialize viewModel of type { context.ViewModel.GetType().Name }. Serialization failed at property { writer.Path }. {GeneralViewModelRecommendations}", ex); | ||
throw new Exception($"Could not serialize static command {description} of type '{ data.GetType().FullName}'. Serialization failed at property { writer.Path }. {GeneralViewModelRecommendations}", ex); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: It will say "static command", even commands. We could just say "Could not serialize result"/"Could not serialize custom data".
{ | ||
context.CommandResult = new PageErrorModel { | ||
Message = clientError.Message | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes the CommandResult
type, which will only cause problems and should be probably forbidden.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This is not good practice.
Co-authored-by: Stanislav Lukeš <[email protected]>
Co-authored-by: Stanislav Lukeš <[email protected]>
…operies are now read-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this looks fine.
Just, could the AddCustomResponseProperty
throw an exception after the data is serialized? (In a similar fashion like AddResource does it)
…xt has been serialized. Test for that.
Implemented an extensible way to deal with results of commands.
The point is to serve use cases when you need to return additional data from static command. One such use case is validation results. Which have up until now been unpractical to use with static commands.
As static commands are used more and more prominently due to their performance advantages. Lack features like validation is more and more painful.
The key point is to:
FilterAttribute
.We refrained from implementing any concrete solution like
ModelState
in the command result. Instead we want to let the programmer define custom solution. This is not only for validation but also telemetry and such.