Error handling in JSONRPC #538

fastbike · 2021-12-14T01:26:31Z

The Index method in the TMVCJSONRPCController has a try/except code block that handles all Exception types.
This means that an application writer is prevented from writing a high level exception handling block that allows custom exception handling as in the normal RESTful mvc application, because all custom error classes are descendants of Exception so get caught.
For instance It prevents an application from being able to log nested exceptions, and also log the call stack using 3rd party code such as Eurekalog.

Issue #191 added a custom exception handler to address this issue for RESTful applications.
I propose a similar pattern for JSONRPC, which would be called from the exception block on the TMVCJSONRPCController.Index method.
Useful objects to pass through are:

the Exception object
the JSON error response, so the application writer can add to the message or change the error code.
the original WebContext, to provide access to request properties
and optionally a boolean variable "ExceptionHandled" set to true, which stops the error from propogating, or could be set to false to reraise the exception.
Thoughts ?

danieleteti · 2021-12-14T09:26:31Z

Good point. Let me think a bit about it.

fastbike · 2021-12-14T10:28:14Z

Thanks, I'll prototype a change in the next few days.

HealthOneNZ · 2021-12-14T19:42:32Z

Will need to include some type of json unit in the core MVCFramework unit if we want to surface the error object - thoughts ?

danieleteti · 2022-03-25T10:34:14Z

Sorry to be late at reply.
@HealthOneNZ Including some json related classes in MVCFramework.pas is not a problem. MVCFramework supports natively JSON-RPC so it makes sense to depends from some sort of JSON stuff; indeed currently JsonDataObjects.pas is included in MVCFramework.pas.
@fastbike do you have some news about this? I'm preparing the official 3.2.2-nitrogen release. If you have something ready we can add to the release with unit tests and will be included in 3.2.2-nitrogen; otherwise we should wait for the next release.

fastbike · 2022-03-25T20:36:36Z

I got as far as adding the following code, it does not yet deal with the JSON error object

...
(* new TProc type  added with the normal exception handler type declaration *)
  TMVCJSONRPCExceptionHandlerProc = reference to procedure(E: Exception;
     //ErrorObj: TMVCJSONObject;
    WebContext: TWebContext; var ExceptionHandled: Boolean);

  TMVCEngine = class(TComponent)
...
(* alter the signature of the method *)
    function PublishObject(const AObjectCreatorDelegate: TMVCObjectCreatorDelegate;
      const AURLSegment: string; ExceptionHandler: TMVCJSONRPCExceptionHandlerProc = nil): TMVCEngine;
...
(* alter the implementation *)
function TMVCEngine.PublishObject(const AObjectCreatorDelegate: TMVCObjectCreatorDelegate;
  const AURLSegment: string; ExceptionHandler: TMVCJSONRPCExceptionHandlerProc = nil): TMVCEngine;
begin
  Result := AddController(TMVCJSONRPCPublisher,
    function: TMVCController
    begin
      Result := TMVCJSONRPCPublisher.Create(AObjectCreatorDelegate(), True, ExceptionHandler);
    end, AURLSegment);
end;
...
(* alter the delegated controllers by adding the param to the constructor, and storing the passed in reference *)
  TMVCJSONRPCController = class(TMVCController)
  private
    fExceptionHandler: TMVCJSONRPCExceptionHandlerProc;
...
  TMVCJSONRPCPublisher = class(TMVCJSONRPCController)
  public
    constructor Create(const RPCInstance: TObject; const Owns: Boolean = True; ExceptionHandler: TMVCJSONRPCExceptionHandlerProc = nil);
        reintroduce; overload;
  end;

(* and the implementation *)
{ TMVCJSONRPCController }
constructor TMVCJSONRPCPublisher.Create(const RPCInstance: TObject; const Owns: Boolean = True; ExceptionHandler:
    TMVCJSONRPCExceptionHandlerProc = nil);
begin
  inherited Create;
  fRPCInstance := RPCInstance;
  fOwsRPCInstance := Owns;
  fExceptionHandler := ExceptionHandler;
end;
....
(* and the dispatch method *)
procedure TMVCJSONRPCController.Index;
var
 ...
  lExceptionHandled: Boolean;
begin
...
      on Ex: Exception do // use another name for exception variable, otherwise E is nil!!
      begin
        lJSONResp := CreateError(lReqID, 0, Ex.Message);
        LogE(Format('[JSON-RPC][CLS %s][MSG "%s"]', [Ex.ClassName, Ex.Message]));
        if Assigned(fExceptionHandler) then
        begin
          lExceptionHandled := true;
          fExceptionHandler(Ex, (*lJSONResp,*) Context, lExceptionHandled);
          if not lExceptionHandled then
            raise;
        end;
      end;
    end; // except
...

I'll take a look at passing through the error JSON object in the next iteration

fastbike · 2022-03-25T21:47:34Z

If I change the signature of the Exception handler, I'll need to add a JSON unit to the uses clause in the MVCFramework.pas file.

This will then bind the whole library whatever library is used e.g. System.Json or JsonDataObjects

Do you have a preference or can this be better handled via conditional compilation ?
You are using jsondataobjects in the MVCFramework.JSONRPC.pas unit.

  TMVCJSONRPCExceptionHandlerProc = reference to procedure(E: Exception;
     ErrorObj: TJSONObject;  // what JSON library should this reference 
    WebContext: TWebContext; var ExceptionHandled: Boolean);

danieleteti · 2022-03-25T22:37:22Z

dmvcframework uses jsondataobject not system.json so it's ok to use jsondataobject

danieleteti · 2022-03-25T22:38:44Z

Why not pass only the relevant properties as string and integer?
JSON-RPC 2.0 specs for error object say:

When a rpc call encounters an error, the Response Object MUST contain the error member with a value that is a Object with the following members:

code
A Number that indicates the error type that occurred.
This MUST be an integer.

message
A String providing a short description of the error.
The message SHOULD be limited to a concise single sentence.

data
A Primitive or Structured value that contains additional information about the error.
This may be omitted.
The value of this member is defined by the Server (e.g. detailed error information, nested errors etc.).

We don't need to pass a "JSON error" there, we can pass just the info to describe it.

https://www.jsonrpc.org/specification#error_object

fastbike · 2022-03-25T23:03:28Z

Why not pass only the relevant properties as string and integer?

The object that is created within the JSONRPC controller encapsulates:
RequestID, ErrorCode, ErrorMessage

If I pass these as individual params, then the framework will need to deal with any changes to them. Also by passing the JSON object, it allows the application developer to add fields to the object which can be used by the front end js developer.

Another alternative would be to pass the JsonObject as a var string, and then parse it back to the lJSONResp variable.

E.g. if the exception passes
{"RequestID":1,"Error":{"code":100,"message":"something went wrong"}}

then the application writer could change it to
{"RequestID":1,"Error":{"code":100,"message":"something went wrong","data":"Some additional info for the front end"}}

This would then be sent back to the client. Apart from some loss of efficiency with the additional conversion from JsonObject to string and back, this would give flexibility, and decouple the main MVCFramework from any particular JSOn library.

What do you think ?

Edited to rename the extra field in the return value (I had missed your change in the comment above)

fastbike · 2022-03-25T23:08:49Z

I'll take a look at the linked in specification (thanks for the link)

fastbike · 2022-03-25T23:18:01Z

So I'm proposing to change signature of the anonymous procedure to

  TMVCJSONRPCExceptionHandlerProc = reference to procedure(E: Exception;
    (*SelectedController: TMVCController;*)
    WebContext: TWebContext;
    var ErrorObj: string;
    var ExceptionHandled: Boolean);

Still not sure if the Controller needs to be passed through though.

And the relevant code change in the JSONRPC call is

        if Assigned(fExceptionHandler) then
        begin
          lExceptionHandled := true;
          lJSONRespString := lJSONResp.ToString;
          fExceptionHandler(Ex,  Context, lJSONRespString, lExceptionHandled);
          if not lExceptionHandled then
            raise;
          lJSONResp.Free;
          lJSONResp := TJsonObject.Parse(lJSONRespString) as TJsonObject;
        end;

with a new local var to allow the string to be passed and altered

fastbike · 2022-03-25T23:57:12Z

Here's a patch file for the JSONRPC unit, the MVCFramework unit just has the changes as above (ext changed to txt to allow upload to github)

MVCFramework.JSONRPC.pas.patch.txt

fastbike · 2022-03-26T03:54:37Z

I've tested this with some of our code, here's an example of the code in the webmodule

  FMVC.PublishObject(
    function: TObject
    begin
      result := TEventFunctions.Create;
    end, '/events',
    procedure(E: Exception; WebContext: TWebContext; var ErrorObj: string; var ExceptionHandled: Boolean)
    var
      Json: TJsonObject;
    begin
      json := TJsonObject.Parse(ErrorObj) as TJsonObject;
      json.O['error'].S['data'] := 'some additional stuff';
      ErrorObj := Json.ToJSON;

      LogExceptionMessages(E, WebContext);
      LogStackTrace(E, WebContext);
    end);

And by raising an exception in the TEventFunctions method I get the following back at the browser

{"jsonrpc":"2.0","id":1,"error":{"code":0,"message":"Error Message","data":"some additional stuff"}}

danieleteti · 2022-03-26T08:38:37Z

Passing a Json object (also as string) could break the protocol because user could not respect the errorobject specs. We need to pass a record by var or something else. Then the framework will recreate the error object using the provided data.

fastbike · 2022-03-26T20:31:57Z

I'd then have to declare the record in the main MVCFramework.pas file, which kind of pollutes that file with JsonRPC related structures. So I'm not keen on that direction.

The specification allows the optional "data" field to be either Primitive (i.e. string or number) or Structured such as a object or array.
So this leads us to either use:

a string that can be parsed to json, or
or a variant such as TJsonDataValueHelper but binds us to to a specific json library, or
TValue which could hold string, integer or structured etc.

Using TValue would require a signature like

  TMVCJSONRPCExceptionHandlerProc = reference to procedure(E: Exception;
    WebContext: TWebContext;
    ErrorCode: Integer;
    ErrorMsg: string;
    var ErrorData: TValue;
    var ExceptionHandled: Boolean);

with corresponding unpacking in the controller code.

danieleteti · 2022-03-26T23:33:03Z

Yes, this could be an approach that I like. Another could be to limit the generic nature of data and define it as TMVCStringDictionary

fastbike · 2022-03-27T00:35:59Z

I'm not sure what a string dictionary would bring, when the spec says that "data" can be either Primitive or Structured.
For example the Ethereum wiki shows the following returned data

{
    code: 3,
    message: 'Execution error',
    data: [{
        code: 102,
        message: 'Innsufficient gas'
    },
    {
        code: 103,
        message: 'Gas limit exceeded'
    }]
}

I'm not sure how you could do that with the dictionary.

danieleteti · 2022-03-27T07:58:56Z

Yes, that's true. A dictionary would simple to implement but not flexible as specs recommends.

fastbike · 2022-03-27T08:35:24Z

On reflection, it is not really much different from the "result" field in the case when there is no error. The spec says "The value of this member is determined by the method invoked on the Server." so there is really no constraint apart from what the server decides to send back.

In the same way the "data" sub field of the "error" field, as per 5.1 on the spec, is open ended and is up to the server to determine. So I do think that TValue, or json formatted string are the best options.

danieleteti · 2022-03-27T09:19:58Z

OK, let's go with the TValue and we'll see how it works.

fastbike · 2022-03-27T18:22:36Z

Ok I've got some code changes to implement this.
I'm using a record that gets passed as a var parameter to the application code in the error handler. This allows the error code to be changed (it is hard coded to 0 at the moment) and the application developer access to the message.
The Data element is a TValue so this can be populated with a variety of types: I've tested it with a simple string and by assigning a TJSONObject.

So the relevant part of MVCFramework.pas looks like

  TMVCJSONRPCExceptionErrorObject = record
    Code: Integer;
    Msg: string;
    Data: TValue;
  end;

  TMVCJSONRPCExceptionHandlerProc = reference to procedure(E: Exception;
    (*SelectedController: TMVCController;*)
    WebContext: TWebContext;
    var ErrorObj: TMVCJSONRPCExceptionErrorObject;
    var ExceptionHandled: Boolean);

...

  TMVCEngine = class(TComponent)
...
    function PublishObject(const AObjectCreatorDelegate: TMVCObjectCreatorDelegate;
      const AURLSegment: string; ExceptionHandler: TMVCJSONRPCExceptionHandlerProc = nil): TMVCEngine;
...

I've left the ref to the controller in there, commented out as it is YAGNI at this stage.

I'll make a diff file for the JsonRPC file changes and attach it to a separate post.

fastbike · 2022-03-27T18:29:23Z

Here's the patch for the MVCFramework.JSONRPC.pas. file

MVCFramework.JSONRPC.pas.patch.txt

fastbike · 2022-03-27T18:37:45Z

I've done the following tests:

Assign a string to the Data field

...
    procedure(E: Exception; WebContext: TWebContext; var ErrObj:  TMVCJSONRPCExceptionErrorObject;
      var ExceptionHandled: Boolean)
    begin
      ErrObj.Data := 'more details';
...

which outputs

{"jsonrpc":"2.0","id":1,"error":{"code":0,"message":"Error Message","data":"more details"}}

A custom class

  TMyclass = class
  private
    FSomeValue: string;
  public
    property SomeValue: string read FSomeValue write FSomeValue;
  end;
...
    procedure(E: Exception; WebContext: TWebContext; var ErrObj:  TMVCJSONRPCExceptionErrorObject;
      var ExceptionHandled: Boolean)
    var
      a: TMyclass;
    begin
      a := TMyclass.Create;
      a.SomeValue := 'abc';
      ErrObj.Data := a;
...

which outputs

{"jsonrpc":"2.0","id":1,"error":{"code":0,"message":"Error Message","data":{"SomeValue":"abc"}}}

In this case the client and server would need to understand how to populate/use the contents of the data field, which is outside the scope of the spec, but is allowed as an extensibility mechanism.

danieleteti · 2022-03-31T14:46:02Z

Thanks David for you contribute. I merged your code and did some slight changes. Please, test from your POV and let me know.
Check this example
C:\DEV\dmvcframework\samples\jsonrpc_with_published_objects\ (client and server).
The 3rd tab in the client contains the new calls

fastbike · 2022-03-31T18:17:51Z

Thanks for the refinement. Although I'm getting an error when the Data object is freed by the framework, I'll look into it.
My publisher code looks like this

  FMVC.PublishObject(
    function: TObject
    begin
      result := TEventFunctions.Create;
    end, '/events',
    procedure(E: Exception; WebContext: TWebContext; var ErrInfo: TMVCJSONRPCExceptionErrorInfo; var ExceptionHandled: Boolean)
    var
      Extra: TJsonObject;
    begin
      (* some local functions
      LogExceptionMessages(E, WebContext);
      LogStackTrace(E, WebContext);
      *)
      // add a json object to the "data" field of the response
      Extra := TJsonObject.Create;
      Extra.S['extra'] := 'some extra data';
      ErrInfo.Data := Extra;
      ExceptionHandled := true;
    end);

The code blows up at

          finally
            if not lJSONRespErrorInfo.Data.IsEmpty then
            begin
              if lJSONRespErrorInfo.Data.IsObjectInstance then
              begin
                 lJSONRespErrorInfo.Data.AsObject.Free; // <<== invalid pointer operation
              end;
            end;

which is caused by

destructor TJsonObject.Destroy;
begin
  Clear;
  FreeMem(FItems);  // <<== error raised here
  FreeMem(FNames);
  //inherited Destroy;
end;

fastbike · 2022-03-31T18:42:37Z

Found it, the assigned Data object is already being freed here

destructor TJSONRPCResponseError.Destroy;
begin
  if not FData.IsEmpty then
  begin
    if FData.IsObjectInstance then
    begin
      FData.AsObject.Free;  //<<== object is freed, so the "owning object" now has a dangling pointer
    end;
  end;
  inherited;
end;

However in the exception handling code that the app developer writes, if the ExceptionHandled parameter remains false, then the framework will not assign the Data field so, the framework is responsible for freeing the item. I will create a patch.

fastbike · 2022-03-31T19:08:02Z

Here's a patch. As ownership (and then freeing) only happens when the ExceptionHandled is true, I've added that to the test.

Also I think when the Data property of the TJSONRPCResponseError class is assigned (it calls SetData), then it should free any existing TObject, so the patch reflects that as well.

MVCFramework.JSONRPC.pas.patch.txt

danieleteti · 2022-04-01T10:54:20Z

Merged your changes. Let me know.

fastbike · 2022-04-01T10:59:08Z

Thanks, looks like we have a solution :)

danieleteti · 2022-04-01T11:02:37Z

Thanks a lot for your support, David

danieleteti assigned danieleteti and fastbike Dec 14, 2021

danieleteti added the enhancement label Dec 14, 2021

danieleteti added a commit that referenced this issue Mar 31, 2022

#538 (WIP)

594b3a3

danieleteti added a commit that referenced this issue Apr 1, 2022

More work about #538 (thanks to https://github.com/fastbike)

c1eee8f

fastbike closed this as completed Apr 1, 2022

danieleteti added this to the 3.2.2-nitrogen milestone Apr 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Error handling in JSONRPC #538

Error handling in JSONRPC #538

fastbike commented Dec 14, 2021 •

edited

Loading

danieleteti commented Dec 14, 2021

fastbike commented Dec 14, 2021

HealthOneNZ commented Dec 14, 2021

danieleteti commented Mar 25, 2022

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

danieleteti commented Mar 25, 2022

danieleteti commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022

fastbike commented Mar 26, 2022

danieleteti commented Mar 26, 2022

fastbike commented Mar 26, 2022 •

edited

Loading

danieleteti commented Mar 26, 2022

fastbike commented Mar 27, 2022 •

edited

Loading

danieleteti commented Mar 27, 2022

fastbike commented Mar 27, 2022 •

edited

Loading

danieleteti commented Mar 27, 2022

fastbike commented Mar 27, 2022 •

edited

Loading

fastbike commented Mar 27, 2022

fastbike commented Mar 27, 2022 •

edited

Loading

danieleteti commented Mar 31, 2022 •

edited

Loading

fastbike commented Mar 31, 2022 •

edited

Loading

fastbike commented Mar 31, 2022 •

edited

Loading

fastbike commented Mar 31, 2022

danieleteti commented Apr 1, 2022

fastbike commented Apr 1, 2022

danieleteti commented Apr 1, 2022

Error handling in JSONRPC #538

Error handling in JSONRPC #538

Comments

fastbike commented Dec 14, 2021 • edited Loading

danieleteti commented Dec 14, 2021

fastbike commented Dec 14, 2021

HealthOneNZ commented Dec 14, 2021

danieleteti commented Mar 25, 2022

fastbike commented Mar 25, 2022 • edited Loading

fastbike commented Mar 25, 2022 • edited Loading

danieleteti commented Mar 25, 2022

danieleteti commented Mar 25, 2022 • edited Loading

fastbike commented Mar 25, 2022 • edited Loading

fastbike commented Mar 25, 2022

fastbike commented Mar 25, 2022 • edited Loading

fastbike commented Mar 25, 2022

fastbike commented Mar 26, 2022

danieleteti commented Mar 26, 2022

fastbike commented Mar 26, 2022 • edited Loading

danieleteti commented Mar 26, 2022

fastbike commented Mar 27, 2022 • edited Loading

danieleteti commented Mar 27, 2022

fastbike commented Mar 27, 2022 • edited Loading

danieleteti commented Mar 27, 2022

fastbike commented Mar 27, 2022 • edited Loading

fastbike commented Mar 27, 2022

fastbike commented Mar 27, 2022 • edited Loading

danieleteti commented Mar 31, 2022 • edited Loading

fastbike commented Mar 31, 2022 • edited Loading

fastbike commented Mar 31, 2022 • edited Loading

fastbike commented Mar 31, 2022

danieleteti commented Apr 1, 2022

fastbike commented Apr 1, 2022

danieleteti commented Apr 1, 2022

fastbike commented Dec 14, 2021 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

danieleteti commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 25, 2022 •

edited

Loading

fastbike commented Mar 26, 2022 •

edited

Loading

fastbike commented Mar 27, 2022 •

edited

Loading

fastbike commented Mar 27, 2022 •

edited

Loading

fastbike commented Mar 27, 2022 •

edited

Loading

fastbike commented Mar 27, 2022 •

edited

Loading

danieleteti commented Mar 31, 2022 •

edited

Loading

fastbike commented Mar 31, 2022 •

edited

Loading

fastbike commented Mar 31, 2022 •

edited

Loading