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

Using Spring4D Collections in ViewData #689

Open
fastbike opened this issue Sep 3, 2023 · 19 comments
Open

Using Spring4D Collections in ViewData #689

fastbike opened this issue Sep 3, 2023 · 19 comments
Assignees
Labels
enhancement open Some team member is working on this

Comments

@fastbike
Copy link
Contributor

fastbike commented Sep 3, 2023

I'm looking at the Spring4D collection classes, in particular the various Lists.

I currently have code in my controllers that fetches a list of objects and adds them to the page to be rendered. However the controller method needs to manage the memory of the list.

procedure TMovieController.GetMoviesPage;
var
  Movies: TObjectList<TMovie>;
begin
  Movies := GetMovieService.ListAll;
  try
    ViewData['Movies'] := Movies;
    LoadView(['Header', 'Movie', 'Footer']);
    RenderResponseStream;
  finally
    Movies.Free;
  end;
end;

If the ViewData would accept an interfaced object, then my code would be simplified removing much of the temporary variable declaration and memory management scaffolding.

procedure TMovieController.GetMoviesPage;
begin
  ViewData['Movies'] := GetMovieService.ListAll;   //returning a  IList<TMovie>, this is from spring.collections.pas
  LoadView(['Header', 'Movie', 'Footer']);
  RenderResponseStream;
end;

The current implementation is looking for a TObject. Can we override it to also accept IInterface and descendants ?

@danieleteti
Copy link
Owner

Let dmvcframework depends from Spring4d would be a huge change and huge dependency. We've to find a different approach.

@danieleteti danieleteti self-assigned this Sep 21, 2023
@danieleteti danieleteti added the open Some team member is working on this label Sep 21, 2023
@fastbike
Copy link
Contributor Author

Probably we could avoid the Spring4D dependency if we can take a Duck Typing approach. I.e. Support Lists by looking for some iteration interface, and Objects by scanning the properties.
I'd need to play around some more to see what is available.

@danieleteti
Copy link
Owner

The unit MVCFramework.DuckTyping.pas already implement lot of Duck Typing oriented code and has been around for years. Maybe with some tweaks and we could use it for ViewData...

@fastbike
Copy link
Contributor Author

I'll add it to my todo list, to investigate.
Although I've probably promised my boss more than I can deliver :(

@fastbike
Copy link
Contributor Author

I've got some code in my toolbox that is a container that can handle both TObject derived instances as well as reference counted IInterface derived instances. As an aside, I've been using it to implement a class helper for TWebContext (so I can can have application specific pseudo properties anywhere the WebContext is available) by binding it to the CustomIntfObject property.

The interface for this container is below - note that it automatically increments and decrements the reference count for IInterface instances to prevent them going out of scope and thus keeps the auto memory management working correctly.

  /// <summary>Helper container for MVC Context auxilliary objects</summary>
  /// <remarks>Supports adding and retrieving both (classic) TObject and (reference counted) IInterface objects
  /// The container will increment the reference count when item is added, and decrement when it is removed
  ///</remarks>
  IWebContextObjectsDictionary = interface
    ['{B70454F5-DD2E-4727-9B3B-D910CEE113AC}']
    procedure Add(const Key: string; const Value: TObject); overload;
    procedure Add(const Key: string; const Value: IInterface); overload;
    function TryGetValue(const Key: string; out Value: TObject): Boolean; overload;
    function TryGetValue(const Key: string; out Value: IInterface): Boolean; overload;
    procedure Remove(const Key: string);
  end;

I could probably refactor this to be more general so it would then be suitable as the container for the ViewData, although as implemented it assumes ownership of the contained objects and will call their destructor as part of its owner destructor (this could be changed though).

@fastbike
Copy link
Contributor Author

The other way is to add an additional ViewData2 container that is generalised for reference counted instances and change the rendering code to iterate over both containers.

@fastbike
Copy link
Contributor Author

Either way will require a change to the TMVCBaseViewEngine class as this takes the AViewModel (type TMVCViewDataObject) in the constructor.
Either we change the type that is passed in, or we add an additional parameter.
Given that this change should be seamless to the user of the Framework (i.e. they should be fairly agnostic about what type of object is passed in) I'd be looking to implement the ViewModel with a container that can handle either type.

@fastbike
Copy link
Contributor Author

OK, so my first naive implementation just stands up a parallel ViewModel that has a TDictionary<string, IInterface> as the storage, and the Mustache renderer takes this as an additional param which it iterates over when preparing the models.

procedure TMVCMustacheViewEngine.PrepareModels;
// snip
begin
  if Assigned(ViewModel) then
  begin
    for DataObj in ViewModel do
    begin
// snip
    end;
  end;
// new code to process the IInterface instances
  if Assigned(ViewModel2) then
  begin
    for DataObj2 in ViewModel2 do
    begin
      lList := TDuckTypedList.Wrap(TObject(DataObj2.Value));
      if lList <> nil then
        lJSON := lSer.SerializeCollection(DataObj2.Value)
      else
        lJSON := lSer.SerializeObject(DataObj2.Value);
      if not lFirst then
        lSJSON := lSJSON + ',';
      lSJSON := lSJSON + '"' + DataObj2.Key + '":' + lJSON;
      lFirst := False;
    end;
  end;
// resume existing code
  if Assigned(ViewDataSets) then
  begin
// snip
  end;
// snip
end;

This renders out a Spring4D list with no references to that framework apart from in my controller unit. So we are not coupling DMVC with Spring4D !
Also I check for memory leaks and there are none - I was a little hesitant in casting the reference counted instance to a TObject so wanted to double check.

I would prefer to propose some changes that preserve the existing interface by modifying the underlying storage so look forward to your feedback when you have time to review.

@fastbike
Copy link
Contributor Author

fastbike commented Sep 26, 2023

Hmm, I wonder if I'm over complicating this LOL.
I've been back in the code and have just cast the Spring framework IList instance to a TObject and then added it to the ViewData.
So that code in the first post of this ticket now becomes

procedure TMovieController.GetMoviesPage;
begin
  // get  IList<TMovie> (from spring.collections.pas) and cast it as a vanilla Delphi object
  ViewData['Movies'] := TObject(GetMovieService.ListAll);   
  LoadView(['Header', 'Movie', 'Footer']);
  RenderResponseStream;
end;
// the Interfaced object returned by the "ListAll" method above will be out of scope by here so its destructor will be called via the reference counting mechanism

My next task will be looking at the MVCFramework.DataSet.Utils unit so I can write some code to output the TDataset as a Spring4D IList<T> instance. Modifying the TDataSetUtils.DataSetToObjectList<T> method to accept a duck typed list would enable standard Delphi lists as well as other lists that exhibit list like behaviour (specifically the ability to Add an object) to be used.

@HealthOneNZ
Copy link

I've found that the TDataSetUtils class has been declared as sealed so this is making it hard to subclass for use with an interfaced list.

I might just clone the unit, unless there is a better idea.

@danieleteti
Copy link
Owner

If there is a good reason we can remove the sealed clause.

@fastbike
Copy link
Contributor Author

fastbike commented Oct 2, 2023

If there is a good reason we can remove the sealed clause.

Sorry, that was me posting from a work account. I'll take a look and see what duck type list changes would make it work with different lists.

@fastbike
Copy link
Contributor Author

fastbike commented Oct 2, 2023

I'm stuck between trying to add "generic" interfaced list functionality - I've looked at IMVCList (used by TDuckTypedList) but this does not support the IMVCList<T> generic functionality.

Or just run with a copy of the unit and rewrite to support Spring4D (so outside of a contribution) to this excellent library.

Option 1 is preferable from a community perspective, option 2 from a get-the-job-done perspective.

@danieleteti
Copy link
Owner

I know the feeling. Writing a framework is not like just "make it work" job.
In this case, we could follow the aproach of sample and contributed units. Then we could find the way to bring that funzionality in the core without adding dependencies.

@fastbike
Copy link
Contributor Author

fastbike commented Oct 2, 2023

Option 2 yields the following - unfortunately due to the lack of a generic version of TDuckTyped list I've need to replicate the DataSetToObjectList on the utility class. It also means I have pulled in reference to the Spring collections rather than being able to use TDuckTypedList.IsWrappedList to decide if the list can be wrapped so the Add method can be accessed.
You'll see I've commented the line where the item to be added to the list is created.
As an aside the inheritance of the class helper hides the method with the same name (overload does not fix the visibility)

unit uDatasetHelpers;

interface

uses
  System.SysUtils,
  System.Classes,
  Data.DB,
  System.Rtti,
  MVCFramework.DataSet.Utils,
  Spring, Spring.Collections, Spring.Collections.Lists;

type

  TDataSetHelper2 = class helper(TDataSetHelper) for TDataSet
    function AsObjectList<T: class, constructor>(CloseAfterScroll: boolean = false; OwnsObjects: boolean = true): IList<T>;
  end;

  TDataSetUtils2 = class // (TDataSetUtils)// sealed
  public
    class procedure DataSetToObjectList<T: class, constructor>(ADataSet: TDataSet; AObjectList: IList<T>;
      ACloseDataSetAfterScroll: boolean = true);
  end;

implementation

uses
  System.Generics.Collections;

{ TDataSetHelper2 }
function TDataSetHelper2.AsObjectList<T>(CloseAfterScroll, OwnsObjects: boolean): IList<T>;
begin
  Result := TCollections.CreateObjectList<T>(OwnsObjects);
  try
    TDataSetUtils2.DataSetToObjectList<T>(Self, Result, CloseAfterScroll);
  except
    Result := nil;
    raise;
  end;
end;

{ TDataSetUtils2 }
class procedure TDataSetUtils2.DataSetToObjectList<T>(ADataSet: TDataSet; AObjectList: IList<T>;
  ACloseDataSetAfterScroll: boolean);
// NB: no functional change here - just a change of signature for the method
var
  Obj: T;
  SavedPosition: TArray<Byte>;
begin
  ADataSet.DisableControls;
  try
    SavedPosition := ADataSet.Bookmark;
    while not ADataSet.Eof do
    begin
      Obj := T.Create; // this requires a generic type
      TDataSetUtils.DataSetToObject(ADataSet, Obj);
      AObjectList.Add(Obj);
      ADataSet.Next;
    end;
    if ADataSet.BookmarkValid(SavedPosition) then
      ADataSet.Bookmark := SavedPosition;
  finally
    ADataSet.EnableControls;
  end;
  if ACloseDataSetAfterScroll then
    ADataSet.Close;
end;

end.

@danieleteti
Copy link
Owner

Did you tried to just hard-cast the interface reference to TObject?

Example:

uses
   MVCFramework.DuckTyping;

{ snip snip snip }

function TWebSiteController.PeopleList: String;
var
  LDAL: IPeopleDAL;
  lPeople: TPeople;
  lPeople2: IInterface;
begin
  LDAL := TServicesFactory.GetPeopleDAL;
  lPeople := LDAL.GetPeople;
  try
    lPeople2 := WrapAsList(LDAL.GetPeople); //just for test, lPeople2 is now an interface
    ViewData['people'] := lPeople;
    ViewData['people2'] :=  TObject(lPeople2); //having already an interface this should be enough
    Result := GetRenderedView(['header', 'people_list', 'footer']);
  finally
    lPeople.Free;
  end;
end;

@sglienke
Copy link

ef6edd5 should already solve this issue - the only thing is that you cannot directly assign any interface to TValue but need to write ViewData['...'] := TValue.From(someInterface);
I don't know though if internally everything is set up to properly work with interfaces inside those TValue that are stored inside the viewdata.

@fastbike
Copy link
Contributor Author

It looks like there have been some major changes prior to the referenced commit, in particular line 236 of the MVCFramework.View.Renderers.Mustache unit cannot be compiled

lSer.TValueToJSONObjectProperty(lJSONModel, DataObj.Key, DataObj.Value, TMVCSerializationType.stDefault, nil, nil);

Is this part of a released branch or still in the development branch ?

@danieleteti
Copy link
Owner

Did you tested this issue in 3.4.2-magnesium-rc1 ?

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

No branches or pull requests

4 participants