Some bug fixes and suggested modifications for mORMot2

Lauri · 2021-01-05 21:48:27

Hi!

1. In mormot.lib.winhttp.pas, procedure HttpApiInitialize:

  if Http.Module <> 0 then
    exit; // already loaded
  mormot.core.os.GlobalLock;
  try
    if Http.Module <> 0 then // remove this
    try

Remove the second check for "Http.Module = 0". Otherwise the API never gets initialized and there will be an AV when starting web server or using httpget.

2. In mormot.orm.rest.pas, function TRestOrm.AsynchBatchStop, add additional check for fRest.Run = nil. Without it one of the tests fails.

  if (self = nil) or
     (fRest.Run = nil) or // add this
     (fRest.Run.BackgroundTimer = nil) or
     (fRest.Run.BackgroundTimer.BackgroundBatch = nil) then
    result := false
  else
    result := fRest.Run.BackgroundTimer.AsynchBatchStop(Table);

3. In mormot.rest.server.pas, class TRestServerURIContext, please make this Error call virtual:

procedure Error(E: Exception; const Format: RawUTF8;
      const Args: array of const; Status: integer = HTTP_BADREQUEST); overload;

That way i can handle output for some exception classes.

4. In mormot.rest.http.server.pas:
- i suggest moving defines COMPRESSSYNLZ and COMPRESSDEFLATE to mormot.defines.inc. Seems to me more logical place to look for those.
- in function TRestHttpServer.Request, i suggest moving the block with check for OnlyJSONRequests below OPTIONS method handler. This way, OPTIONS request will work even when OnlyJSONRequests is set. Thus allowing browser to send requests with content-type "application/json". Like so:

   ...
    else
      result := HTTP_BADREQUEST
// move this: 
//  else if (Ctxt.Method = '') or
//          (OnlyJSONRequests and
//           not IdemPChar(pointer(Ctxt.InContentType), JSON_CONTENT_TYPE_UPPER)) then
//    // wrong Input parameters or not JSON request: 400 BAD REQUEST
//    result := HTTP_BADREQUEST
// from here... 
  else if Ctxt.Method = 'OPTIONS' then
  begin
    // handle CORS
    if fAccessControlAllowOrigin = '' then
      Ctxt.OutCustomHeaders := 'Access-Control-Allow-Origin:'
    else
    begin
      FindNameValue(Ctxt.InHeaders, 'ACCESS-CONTROL-REQUEST-HEADERS:', headers);
      Ctxt.OutCustomHeaders := 'Access-Control-Allow-Headers: ' + headers;
      ComputeAccessControlHeader(Ctxt);
    end;
    result := HTTP_NOCONTENT;
  end
// to here:
  else if (Ctxt.Method = '') or
          (OnlyJSONRequests and
           not IdemPChar(pointer(Ctxt.InContentType), JSON_CONTENT_TYPE_UPPER)) then
    // wrong Input parameters or not JSON request: 400 BAD REQUEST
    result := HTTP_BADREQUEST
  else
  ....

5. Seems to me that implementations for TRestServerRoutingREST and TRestServerRoutingJSON_RPC are switched. The header comments are correct but implementation for REST is clearly checking for JSON_RPC mode of method calling and vice-versa.

6. Interface methods return output parameters without commas like so:
{"n1":2"n2":3"text":"test text"}
instead of:
{
"n1": 2,
"n2": 3,
"text": "test text"
}
I have set
ResultAsJSONObject := True;
ResultAsJSONObjectWithoutResult := True;

7. Defining an interface with function returning TServiceCustomAnswer will cause AV when calling TInterfaceFactory.RegisterInterfaces.
in mormot.core.base, function TSynTempBuffer.InitZero, there is a call "Init(ZeroLen - 16)" but ZeroLen is 12 in this case. That results in a negative value and buf getting set to nil. Next FillCharFast will fail.

This is based on source archive taken on 12.24. So maybe you already changed some of those things..

ab · 2021-01-06 09:29:35

Yesterday, I already fixed some of the issues (5-7 at least).

I will look forward to the other points.

Thanks a lot for the feedback!
It is a very good sign that you find yourself your way into the new source code base!

ab · 2021-01-06 10:25:18

I tried to implement your proposals.

Please check https://github.com/synopse/mORMot2/comm … 7ddbdd237b

Thanks again!

Lauri · 2021-01-06 16:53:58

Thank you ab!
I've checked now and all works perfectly except for a corner case with issue 7.

Having such (incorrect?) implementation will cause an AV in mormot.core.interfaces line 7146 (AddJson) because fValues is nil.

function TSomeInterfaceMethodObject.Test1: TServiceCustomAnswer;
begin
  Result.Status := 200;
  Result.Content := 'seems to work';
end;

These work fine though:

function TSomeInterfaceMethodObject.Test2: TServiceCustomAnswer;
begin
  Result.Header := 'Content-type: application/json';
  Result.Status := 200;
  Result.Content := '{"x": "seems to work"}';

function TSomeInterfaceMethodObject.Test3: TServiceCustomAnswer;
begin
  Result.Header := BINARY_CONTENT_TYPE;
  Result.Status := 200;
  Result.Content := 'seems to work';
end;

ab · 2021-01-06 18:07:48

In mORMot 1.18, Header is mandatory.

I have made Header optional - setting JSON content as default.
And ensure that binary serialization is used for SOA if no RTTI is available.

Lauri · 2021-01-07 15:37:27

I propose a new define:

{$define ONLYUSEHTTPSYS}
// disable automatic failover to Http socket server when http.sys based server fails

and implementation in mormot.rest.http.server.pas, in exception handler, around line 665 add:

    on E: Exception do
    begin
      log.Log(sllError, '% for % % at%  -> fallback to socket-based server',
        [E, ToText(aUse)^, fHttpServer, ServersRoot], self);
      FreeAndNil(fHttpServer); // if http.sys initialization failed
      {$ifdef ONLYUSEHTTPSYS}
      raise;
      {$endif}
    end;

This way i can report the problem to user and abort startup. Silent fallback is not desired when SSL is required.

Lauri · 2021-01-07 16:20:45

It seems that THttpApiServer.RemoveUrl does not remove a registered URL previously added by AddUrl. Http.RemoveUrlFromUrlGroup(fUrlGroupID, pointer(uri), 0) returns 0 (success). Yet "netsh http show urlacl" shows the registration remains.
I want to add a config option to my project to allow the user to pick either HTTP or HTTPS so removing the other one is necessary.
I do have a workaround to use "netsh http delete urlacl ...".

ab · 2021-01-07 17:24:16

I preferred to add new useHttpApiOnly and useHttpApiRegisteringURIOnly options and not a conditional.
Conditionals are to enable/disable full features at project level, not changing the behavior.

Check https://github.com/synopse/mORMot2/comm … 8d9af41d75

Lauri · 2021-01-07 17:30:05

Of course, even better this way!

ab · 2021-01-07 17:35:30

About RemoveUrlFromUrlGroup() I am afraid it may have been the case even with mORMot 1.18...

mORMot Open Source

#1 2021-01-05 21:48:27

Some bug fixes and suggested modifications for mORMot2

#2 2021-01-06 09:29:35

Re: Some bug fixes and suggested modifications for mORMot2

#3 2021-01-06 10:25:18

Re: Some bug fixes and suggested modifications for mORMot2

#4 2021-01-06 16:53:58

Re: Some bug fixes and suggested modifications for mORMot2

#5 2021-01-06 18:07:48

Re: Some bug fixes and suggested modifications for mORMot2

#6 2021-01-07 15:37:27

Re: Some bug fixes and suggested modifications for mORMot2

#7 2021-01-07 16:20:45

Re: Some bug fixes and suggested modifications for mORMot2

#8 2021-01-07 17:24:16

Re: Some bug fixes and suggested modifications for mORMot2

#9 2021-01-07 17:30:05

Re: Some bug fixes and suggested modifications for mORMot2

#10 2021-01-07 17:35:30

Re: Some bug fixes and suggested modifications for mORMot2

Board footer