-
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
Changes from 14 commits
c9e9560
9b164ee
35744dd
f13d1e4
b921c1c
977696a
2de91de
a0f37a5
6ba6afa
05ca21a
101a60e
5784e12
48de474
fdcb359
11194c3
3ab69eb
02fdf6a
d8eb1dc
6435964
42b98ea
b8a7bad
ae0d2bd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -59,7 +59,7 @@ public string SerializeViewModel(IDotvvmRequestContext context) | |
/// <summary> | ||
/// Builds the view model for the client. | ||
/// </summary> | ||
public void BuildViewModel(IDotvvmRequestContext context) | ||
public void BuildViewModel(IDotvvmRequestContext context, object commandResult) | ||
{ | ||
// serialize the ViewModel | ||
var serializer = CreateJsonSerializer(); | ||
|
@@ -121,6 +121,9 @@ public void BuildViewModel(IDotvvmRequestContext context) | |
} | ||
// TODO: do not send on postbacks | ||
if (validationRules?.Count > 0) result["validationRules"] = validationRules; | ||
|
||
if (commandResult != null) result["commandResult"] = WriteCommandData(commandResult, serializer, "result"); | ||
AddCustomPropertiesIfAny(context, serializer, result); | ||
|
||
context.ViewModelJson = result; | ||
} | ||
|
@@ -140,18 +143,33 @@ public string BuildStaticCommandResponse(IDotvvmRequestContext context, object r | |
UsedSerializationMaps = new HashSet<ViewModelSerializationMap>() | ||
}; | ||
serializer.Converters.Add(viewModelConverter); | ||
var writer = new JTokenWriter(); | ||
var response = new JObject(); | ||
response["result"] = WriteCommandData(result, serializer, "result"); | ||
AddCustomPropertiesIfAny(context, serializer, response); | ||
return response.ToString(JsonFormatting); | ||
} | ||
|
||
private static void AddCustomPropertiesIfAny(IDotvvmRequestContext context, JsonSerializer serializer, JObject response) | ||
{ | ||
if (context.CustomResponseProperties != null && context.CustomResponseProperties.Count > 0) | ||
{ | ||
response["customProperties"] = WriteCommandData(context.CustomResponseProperties, serializer, "custom properties"); | ||
} | ||
} | ||
|
||
private static JToken WriteCommandData(object data, JsonSerializer serializer, string description) | ||
{ | ||
var writer = new JTokenWriter(); | ||
try | ||
{ | ||
serializer.Serialize(writer, result); | ||
serializer.Serialize(writer, data); | ||
} | ||
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 commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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". |
||
} | ||
response["result"] = writer.Token; | ||
return response.ToString(JsonFormatting); | ||
|
||
return writer.Token; | ||
} | ||
|
||
protected virtual JsonSerializer CreateJsonSerializer() => DefaultSerializerSettingsProvider.Instance.Settings.Apply(JsonSerializer.Create); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
using DotVVM.Framework.Hosting; | ||
using DotVVM.Framework.Runtime.Filters; | ||
using DotVVM.Framework.ViewModel; | ||
|
||
namespace DotVVM.Samples.Common.ViewModels.FeatureSamples.CustomResponseProperties | ||
{ | ||
public class PageErrorModel | ||
{ | ||
public string Message { get; set; } | ||
} | ||
public class ClientExceptionFilterAttribute : ExceptionFilterAttribute | ||
{ | ||
protected override Task OnCommandExceptionAsync(IDotvvmRequestContext context, ActionInfo actionInfo, Exception exception) | ||
{ | ||
if (exception is UIException clientError) | ||
{ | ||
context.AddCustomResponseProperty("validation-errors",new PageErrorModel { | ||
Message = clientError.Message | ||
}); | ||
context.AddCustomResponseProperty("Message", "Hello there"); | ||
|
||
context.IsCommandExceptionHandled = true; | ||
} | ||
return Task.FromResult(0); | ||
} | ||
} | ||
public class UIException : Exception | ||
{ | ||
public UIException(string message) : base(message) | ||
{ | ||
} | ||
} | ||
public class SimpleExceptionFilterViewModel : DotvvmViewModelBase | ||
{ | ||
[AllowStaticCommand] | ||
[ClientExceptionFilter] | ||
public static void StaticCommand() | ||
{ | ||
throw new UIException("Problem!"); | ||
} | ||
|
||
[ClientExceptionFilter] | ||
public void Command() | ||
{ | ||
throw new UIException("Problem!"); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
@viewModel DotVVM.Samples.Common.ViewModels.FeatureSamples.CustomResponseProperties.SimpleExceptionFilterViewModel, DotVVM.Samples.Common | ||
@import model = DotVVM.Samples.Common.ViewModels.FeatureSamples.CustomResponseProperties.SimpleExceptionFilterViewModel | ||
|
||
<html> | ||
<head> | ||
</head> | ||
<body> | ||
<div> | ||
<dot:Button Click="{staticCommand: model.StaticCommand()}" data-ui="staticCommand" Text="Static Command Test" /> | ||
<dot:Button Click="{command: Command()}" Text="Command Test" data-ui="command" /> | ||
<dot:Button onclick="javascript: clearTexts()" Text="Clear" data-ui="clear"></dot:Button> | ||
</div> | ||
<p> | ||
<span data-ui="customProperties"></span><br /> | ||
</p> | ||
<dot:InlineScript> | ||
function clearTexts(){ | ||
var customProperties = document.querySelector('[data-ui="customProperties"]'); | ||
customProperties.innerText = ""; | ||
} | ||
dotvvm.events.staticCommandMethodInvoked.subscribe(e => { | ||
var customPropertiesInput = document.querySelector('[data-ui="customProperties"]'); | ||
customPropertiesInput.innerText = e.serverResponseObject.customProperties.Message; | ||
}); | ||
|
||
dotvvm.events.postbackResponseReceived.subscribe(e => { | ||
var customPropertiesInput = document.querySelector('[data-ui="customProperties"]'); | ||
customPropertiesInput.innerText = e.serverResponseObject.customProperties.Message; | ||
}); | ||
</dot:InlineScript> | ||
</body> | ||
</html> | ||
|
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 typeDotvvmPostbackError
.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 indotvvm.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.