search

thotsaphon / delphi-orm

Automatically exported from code.google.com/p/delphi-orm
0 stars 0 forks source link

TDormUOW inconsistencies #24

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
TDormUOW class seems quite dangerous to use.

Problem 1. FUOWDelete object ownership is not very good idea:

constructor TdormUOW.Create;
begin
  ...
  FUOWDelete := NewList(true); // this MUST delete also the objects // I think this is right only when Session.Save(MyUOW) is called and when DB-level transactions are not used
end;

destructor TdormUOW.Destroy;
begin
  ...
  FUOWDelete.Free; // This will free all objects, registered for delete, even if there was no call to Session.Save(MyUOW);
  ...
end;

procedure TdormUOW.Clear;
begin
  ...
  FUOWDelete.Clear; // this MUST delete also the objects. WHY???
end;

Solution 1.
FUOWDelete list should not "explicitly" own objects, registered for delete. 
These objects should be extracted from this list and freed inside the call to 
Session.Save(MyUOW).
Solution 2.
TDormUOW should call .Cancel before Clear and Destroy

Problem 2. FUOWDelete.Cancel seems incomplete, because it clears only 
"Delete-list", but not clears Insert and Update lists.

Original issue reported on code.google.com by alisov.a...@gmail.com on 5 Jan 2012 at 7:45

GoogleCodeExporter commented 9 years ago 
TdormUOW is intended to use only with method Session.Save(UOW).

In a big project in my company, each time we want to use the UOW, we need to 
clear also the objects in the "delete" list. So this is the standard behaviour. 
However, I'm agree with your general idea. 
So, I suggest to put a default parameter "OwnDeleteObjects" in the UOW 
constructor.

What do you think about?

Original comment by daniele....@gmail.com on 5 Jan 2012 at 8:43

GoogleCodeExporter commented 9 years ago 
>>TdormUOW is intended to use only with method Session.Save(UOW).

I was talking about the case
UOW := TdormUOW.Create;
try
  UOW.AddDelete(SomeObj1);
  UOW.AddDelete(SomeObj2);

  -- Some exception happens here --

  Session.Save(UOW); // Is not executed because of exception
finally
  UOW.Free; // Oops, SomeObj1 and SomeObj2 are freed
end;

Original comment by alisov.a...@gmail.com on 5 Jan 2012 at 9:08

GoogleCodeExporter commented 9 years ago 
I got the point... 
So, we could only (in the "delete") set the IOD to nullvalue ?
What do you think about?
Should be possibile to do the following

freedeleted := true;
UOW := TdormUOW.Create;
try
  UOW.AddDelete(SomeObj1);
  UOW.AddDelete(SomeObj2);
  try
  -- some exception happends here
  except
    freedeleted := false;
  end;
  Session.Save(UOW); // Is not executed because of exception  
finally
  if freedeleted then
    UOW.FreeDeletedObject; //??  let user decide or not
  UOW.Free; // Oops, SomeObj1 and SomeObj2 are freed
end;

if I dont call UOW.FreeDeleted I can still write

Session.Insert(SomeObj1); //Inserted as new object because OID is set to 
nullvalue

what do you think about?

Original comment by daniele....@gmail.com on 5 Jan 2012 at 9:40

GoogleCodeExporter commented 9 years ago 
FreeDeletedObjects parameter (which you suggested, I just gave it another name 
:) ) is of course good and you can add it, but it is good for a bit another 
reason (not for the reason I started this talk).

I was trying to avoid freeing the objects when they are registered for delete 
through UOW, but Session.Save is not called.
In this case you can add to TDormUOW
property Executed: Boolean read FExecuted write SetExecuted;
This property is initially set to False and is set to True by Session.Save(UOW) 
call. In TDormUOW.Clear/Destroy calls you can check, whether TDormUOW is 
Executed (i.e. applied) and free deleted objects if FreeDeletedObjects=True AND 
TDormUOW.Executed=True.

Diving more deeply to TDormUOW, I've found another example of dangerous 
behavior:

function TdormUOW.AddDelete(Obj: TObject): TdormUOW;
...
  if FUOWInsert.IndexOf(Obj) > -1 then
  begin
    o := FUOWInsert.Extract(Obj);
    FreeAndNil(o); // This is also not very good, but I understand why you need this
  end
...

It seems that I can't suggest any good variants for changing current TDormUOW 
behavior, because I don't know use cases for this class. The one thing that 
scares me most is that TDormOUW CAN DESTROY my object. This is not good pattern 
(IMHO), because I suppose my objects (if they are not refcounted interfaces) 
already have some owner (usually the owner of the object is somehow set right 
after the moment of object's creation, because I don't want to have memory 
leaks). So, if UOW destroys my object I will get an AV when the real owner of 
the object will try to destroy the object one more time.

Original comment by alisov.a...@gmail.com on 5 Jan 2012 at 10:16

GoogleCodeExporter commented 9 years ago 
Oops, I wrote my comment 4 before reading to your comment 3 :)
You have almost the same idea. The only difference is to add "Executed" 
property inside TdormUOW, which will be set to True by Session.Save(UOW) call.

Original comment by alisov.a...@gmail.com on 5 Jan 2012 at 10:20

GoogleCodeExporter commented 9 years ago 
In your example I would prefer the following pattern:

UOW := TdormUOW.Create;
try
  UOW.AddDelete(SomeObj1);
  UOW.AddDelete(SomeObj2);
  Session.Save(UOW);     
  UOW.FreeDeletedObject; // This is executed only if Session.Save succeeds
finally
  UOW.Free;
end;

But still I can't say this is good, because it is not clear to me, how SomeObj1 
and SomeObj2 are created and why they don't initially have an owner.

Original comment by alisov.a...@gmail.com on 5 Jan 2012 at 10:30

GoogleCodeExporter commented 9 years ago

Original comment by daniele....@gmail.com on 29 Jan 2012 at 5:52

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision 8d39f0ca56f5.

Original comment by daniele....@gmail.com on 30 Jan 2012 at 2:26