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

Error handling in JSONRPC #538

Closed
fastbike opened this issue Dec 14, 2021 · 30 comments
Closed

Error handling in JSONRPC #538

fastbike opened this issue Dec 14, 2021 · 30 comments
Assignees

Comments

@fastbike
Copy link
Contributor

fastbike commented Dec 14, 2021

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

Good point. Let me think a bit about it.

@fastbike
Copy link
Contributor Author

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

@HealthOneNZ
Copy link

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

@danieleteti
Copy link
Owner

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
Copy link
Contributor Author

fastbike commented Mar 25, 2022

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
Copy link
Contributor Author

fastbike commented Mar 25, 2022

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

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

@danieleteti
Copy link
Owner

danieleteti commented Mar 25, 2022

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
Copy link
Contributor Author

fastbike commented Mar 25, 2022

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
Copy link
Contributor Author

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

@fastbike
Copy link
Contributor Author

fastbike commented Mar 25, 2022

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

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
Copy link
Contributor Author

fastbike commented Mar 26, 2022

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

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
Copy link
Contributor Author

fastbike commented Mar 27, 2022

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

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

@fastbike
Copy link
Contributor Author

fastbike commented Mar 27, 2022

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

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

@fastbike
Copy link
Contributor Author

fastbike commented Mar 27, 2022

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
Copy link
Contributor Author

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

MVCFramework.JSONRPC.pas.patch.txt

@fastbike
Copy link
Contributor Author

fastbike commented Mar 27, 2022

I've done the following tests:

  1. 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"}}
  1. 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 added a commit that referenced this issue Mar 31, 2022
@danieleteti
Copy link
Owner

danieleteti commented Mar 31, 2022

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
Copy link
Contributor Author

fastbike commented Mar 31, 2022

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
Copy link
Contributor Author

fastbike commented Mar 31, 2022

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
Copy link
Contributor Author

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

Merged your changes. Let me know.

@fastbike
Copy link
Contributor Author

fastbike commented Apr 1, 2022

Thanks, looks like we have a solution :)

@fastbike fastbike closed this as completed Apr 1, 2022
@danieleteti
Copy link
Owner

Thanks a lot for your support, David

@danieleteti danieleteti added this to the 3.2.2-nitrogen milestone Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants