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

TMVCRequestParamsTable to handle TValue #208

Open
fastbike opened this issue Apr 11, 2019 · 14 comments
Open

TMVCRequestParamsTable to handle TValue #208

fastbike opened this issue Apr 11, 2019 · 14 comments
Assignees
Labels
help wanted open Some team member is working on this

Comments

@fastbike
Copy link
Contributor

The current version of the Context.Request.Params table is defined as TDictionary<string, string> so cannot contain the full range of types that Delphi is capable of.

Changing this to TDictionary<string, TValue> allows a wider range of types to be created and dispatched to Controller methods.

The implementing class is TMVCRequestParamsTable which has been changed in this Pull Request.

  • A helper record TMVCRequestParam handles the management of state of Params from string (when parsed from the URL) to TValues when bound to the controller method's parameters.
  • A variety of constructors allows text or rich types to be added as request params
  • Current types handled presently are Int, Int64, Float, String, TDateTime, Boolean, Interface, Object.
  • A utility class TMVCParamConverter handles the actual conversions from string representations to TValue

Updated Files are attached (including new unit test suite) along with a sample project, and a Postman test collection to exercise the sample project.

@fastbike
Copy link
Contributor Author

GitHub_PR_208.zip

@fastbike
Copy link
Contributor Author

As a side effect the TMVCRequestParamsTable now has an overridden constructor which performs case insensitive comparisons of param keys, so the case matching between Paths params [e.g. /Patient/{$param) ] and controller method params [e.g GetPatient(PARAM: string) ] works regardless of case sensitivity

@danieleteti
Copy link
Owner

Quite nice addition. Good Job! However, the changes has been made over a very old unit compared to the current one in the repo. This makes difficult the merge phase. Could you reapply your changes on the current version of the units? Then your changes and your tests will be integrated. Thank you

@danieleteti danieleteti added enhancement open Some team member is working on this task labels Apr 16, 2019
@fastbike
Copy link
Contributor Author

Changes reapplied to the current base.

Github_PR_208_Take2.zip

@danieleteti
Copy link
Owner

I did a deeper investigation on your changes. I'm not sure to have got the point about your intentions. What your code does that procedure TMVCEngine.FillActualParamsForAction doesn't do already?
It is interesting the possibility to change the parameters before inject them into the action, however considering that we are taking about only url_mapped_params, having the possibility to pass complex object on the url is not a priority for the most application, and where it is needed it is far simple to just create the object into the action code. Did I miss something?

@fastbike
Copy link
Contributor Author

I'll take another look and provide some detailed examples as to why I have refactored the FillActualParamsForAction code into its own helper class - later though :)

@danieleteti
Copy link
Owner

Any news on this?

@fastbike
Copy link
Contributor Author

fastbike commented Jul 8, 2019

My bad. I have made the change to my own fork and have it in a production app so need to extract some examples from that app to show how it allows me to write less code in controllers.

@fastbike
Copy link
Contributor Author

I've finally circled back to this request - to try to share the code changes which I've been using on a private fork of the framework for 18 months..

The main justification for this change is that the parameters to Controller Action implementation methods can be richer than strings which are parsed out of the URL, cookies etc. With this change it is possible to write Controller methods that contain parameters typed for an instance of a TObject/IInterface class. The actual instantiation can be handled by middleware in the BeforeControllerAction method *.

For example, the controller method below has the following parameters automatically populated:

  • ResourceId is populated via the normal parameter mapping - in this case from the position in the request URL path,
  • LocationId is auto populated from the OAuth token passed on the request header (via some middleware),
  • Resource instance is auto created from the XML body of the request (via some other middleware),
  • ConditionalUpdateSearchTerm is auto populated/validated from the URL (via a third piece of middleware) - in this case by parsing the request query string
    [MVCPath('/($ResourceId)')]
    [MVCHTTPMethod([httpPUT])]
    procedure AddMedicationDispense(ResourceID, LocationId: string; Resource: IFHIRMedicationDispense; ConditionalUpdateSearchTerm: string);

This ability to add custom middleware that can automatically instantiate and pre-populate parameters before they hit the controller action is really useful where you have a project with many controllers, each with many methods that expect a fully instantiated object. It means you do not have to call a helper function in each action which is a real plus for developer productivity and code maintainability.
In the large project we are working on, we currently have 15 controllers each with 5 methods using this parameter mapping. Our roadmap has another 15 controllers to add, so that is 150 calls to a helper class we do not have to write and maintain.

I acknowledge that this will be a breaking change as the signature for params changes from string to TValue. However these changes are usually limited to a few controller actions and a new property "ParamsAsString" is provided to provide the string representation. As a breaking change I would suggest a major release such as 4.x

Now that I have synchronised 3.2.2 with our code base I will prepare and upload a code diff. This has simplified the changes.

  • I could share some sample code showing how the middleware maps, and instantiates objects.

@fastbike
Copy link
Contributor Author

fastbike commented Aug 16, 2021

OK I'm having trouble with the pull request - looks like I would need to fork the project on github. So I've attached a zip of the changed files instead (just two)

  1. I have simplified and refactored my original approach so that Params remain as strings but they are also available via a new property as a TValue. This removes most of the breaking changes (a good thing) while allowing the additional functionality of the TValue type to be accessed.

  2. Another change is that the TDictionary used to match the params is no longer case sensitive. This fits with the nature of Pascal (identifiers are NOT case sensitive) so prevents annoying problems from the Delphi code formatter changing the case of params !

  3. The third change is to extract the code (TMVCEngine.GetActualParam) that does the conversion of a string value to a typed value into it's own helper class (TMVCParamConverter).
    This makes it available for middleware to use, so the auto instantiation of complex types can be handled by custom middleware (see the example in my previous post). In future I would like to extend this so it can also handle more types such as enumerated types (just booleans are handled currently).

  4. The last change is to get TMVCEngine.FillActualParamsForAction looking in the ParamsTable to see if a non empty TValue can be matched before reverting to the URL mapped parameters

DMVC_208.zip

@fastbike
Copy link
Contributor Author

I did a deeper investigation on your changes. I'm not sure to have got the point about your intentions. What your code does that procedure TMVCEngine.FillActualParamsForAction doesn't do already? It is interesting the possibility to change the parameters before inject them into the action, however considering that we are taking about only url_mapped_params, having the possibility to pass complex object on the url is not a priority for the most application, and where it is needed it is far simple to just create the object into the action code. Did I miss something?

The code changes I want allow for an object to be instantiated from the Body of a Request (e.g. by some custom middleware) and then be auto injected into the Action as a parameter.

I have a project with 17 controllers, most of which have at least 6 actions that take a fully instantiated object that has been created from the body of the request. This saves me having to call a method to parse and create those objects in every single action, which is repetitive and potentially error prone. Here's an example of a controller action ...

  [MVCPath('/FHIR/AllergyIntolerance')]
  TAllergyIntoleranceController = class(TClinicalController)
  public
    [MVCPath(''), MVCPath('/'), MVCHTTPMethod([httpPOST])]
    [MVCConsumes(TFHIRConstants.XMLContentType), MVCProduces(TFHIRConstants.XMLContentType)]
    procedure AddAllergyIntolerance(LocationId: string; Resource: IFHIRAllergyIntolerance; ConditionalCreateSearchTerm: string);

This would be called by a client with something like

POST http://example.com/FHIR/AllergyIntolerance?identifier=EHRKey|F91AEFB6-6DC6-4DEE-90E0-2C73964BC9F0

<?xml version="1.0" encoding="UTF-8"?>
<AllergyIntolerance xmlns="http://hl7.org/fhir">
  <id value="example"/>
  <identifier>
    <system value="http://acme.com/ids/patients/risks"/>
    <value value="49476534"/>
  </identifier>
  <clinicalStatus>
    <coding>
      <system value="http://terminology.hl7.org/CodeSystem/allergyintolerance-clinical"/>
      <code value="active"/>
      <display value="Active"/>
    </coding>
  </clinicalStatus>
  <type value="allergy"/>
  <category value="food"/>
  <criticality value="high"/>
  <code>
    <coding>
      <system value="http://snomed.info/sct"/>
      <code value="227493005"/>
      <display value="Cashew nuts"/>
    </coding>
  </code>
  <patient>
    <reference value="Patient/example"/>
  </patient>
  <onsetDateTime value="2004"/>
</AllergyIntolerance>

The middleware has a factory that will create an object from a concrete class that implements the IFHIRAllergyIntolerance interface.
Incidentally it also populates the LocationId from a value passed in the authentication token, and ConditionalCreateSearchTerm param is auto populated from the query on the URL.

The key to getting this to work is having the TMVCRequestParamsTable class look like this

  TMVCRequestParamsTable = class(TDictionary<string, TValue>)
  public
    constructor Create;
  end;

Also the constructor has been overridden so we can support case insensitive param names.

I'll see if I can create a fork on github and show you the changes we are currently running with.

@HealthOneNZ
Copy link

I've just reconciled the Magnesium RC with the fork we made (5 years ago) to support TValue in the automagically bound Controller Action parameters. I will tidy up some of our uses cases and submit a PR as I think the code change I've made significantly simplifies building reasonable complicated applications.

E.g. Our main healthcare API application, now has 19 controllers, each with around 6 methods - so 114 methods, that benefit from automatic instantiation of objects via a custom middleware. And we have more planned.
The code typically looks like this

    procedure Update(ResourceID, LocationId: string; Resource: IFHIRObservation);

Our middleware automatically creates the Resource object from the request body, after running some cleanup and business rules to ensure the request body meets desired characteristics. The underlying code is very similar to how the MVCInjectAttribute creates an interfaced object.

// our code which gets called from the middleware
class function TParamUtils.SetResourceParam(ResourceType: string; Body: string; ParamKind: TRttiType): TValue;
var
  ctx: TRttiContext;
  InterfaceType: TRttiInterfaceType;
  Intf: IFHIRResource;
begin
  ctx := TRttiContext.Create;
  try
    InterfaceType := ctx.FindType(ParamKind.QualifiedName) as TRttiInterfaceType;

    { remove BOM }
    if (Copy(Body, 1, 3) = #$EF#$BB#$BF) then
      Body := Copy(Body, 4);

    (* NB: hard cast via Supports is needed to ensure that the object can be matched by IID  later on *)
    if Supports(FHIRResourceFactory.CreateResource(ResourceType, Body), InterfaceType.GUID, Intf) then
    begin
      TValue.Make(@Intf, InterfaceType.Handle, Result);
    end
    else
      raise EFHIRExceptionClient.CreateFmt('Could not cast resource as %s', [ParamKind.QualifiedName]);
  finally
    ctx.Free;
  end;
end;


// cf the DMVC code
        lIntf := AContext.ServiceContainerResolver.Resolve(AActionFormalParams[I].ParamType.Handle, lInjectAttribute.ServiceName);
        Supports(lIntf, AActionFormalParams[I].ParamType.Handle.TypeData.GUID, lOutIntf);
        TValue.Make(@lOutIntf, AActionFormalParams[I].ParamType.Handle, AActualParams[I]);

In our code, we create the Resource object via a factory, where the resource types and implementing classes have been pre-registered. And for this action/method we also populate the LocationId parameter with a value extracted from the JWT authentication token submitted with the request. All of this happens transparently without having to markup the controller code, or run an inherited method inside of the method.

In addition our middleware automatically scans for registered Param Names (e.g. LocationID, ConditionalUpdateSearchTerm, SortOrder, SearchQuery, etc), figures out where to get the parameter vale from and sets the associated value.

This makes our code a lot faster to write and improves the code quality as the auto populating is centralised, can be tested onec, and is never omitted by a team member in a hurry.

@danieleteti
Copy link
Owner

Very nice. However, did you check the dependency injection container with automatic injection available in modern dmvc?

You can register services using something like the following:

unit Unit6;

interface

uses
  Unit5,
  MVCFramework.Container, System.Generics.Collections;

type
  IPeopleService = interface
    ['{F1866213-A1F6-494A-895F-12833BAAB3A0}']
    function GetAll: TObjectList<TPerson>;
  end;

  TPeopleService = class(TInterfacedObject, IPeopleService)
  protected
    function GetAll: TObjectList<TPerson>;
  end;

procedure RegisterServices(Container: IMVCServiceContainer);

implementation

procedure RegisterServices(Container: IMVCServiceContainer);
begin
  Container.RegisterType(TPeopleService, IPeopleService, TRegistrationType.SingletonPerRequest);
  // Register other services here
end;

function TPeopleService.GetAll: TObjectList<TPerson>;
begin
  Result := TObjectList<TPerson>.Create;
  Result.AddRange([
    TPerson.Create(1, 'Henry', 'Ford', EncodeDate(1863, 7, 30)),
    TPerson.Create(2, 'Guglielmo', 'Marconi', EncodeDate(1874, 4, 25)),
    TPerson.Create(3, 'Antonio', 'Meucci', EncodeDate(1808, 4, 13)),
    TPerson.Create(4, 'Michael', 'Faraday', EncodeDate(1867, 9, 22))
  ]);
end;

And then service can be injected (recursively!) into you actions and services.

    //Sample CRUD Actions for a "People" entity
    [MVCPath('/people')]
    [MVCHTTPMethod([httpGET])]
    function GetPeople([MVCInject] PeopleService: IPeopleService): IMVCResponse;

@danieleteti danieleteti self-assigned this Dec 19, 2024
@HealthOneNZ
Copy link

That's my next task :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted open Some team member is working on this
Projects
None yet
Development

No branches or pull requests

3 participants