I am trying to check the googleapiconv "TGoogleRestDescription" object contents (which is descended from restbase TBaseObject) by using "SaveToJSON". But it always saves an empty object, even when the properties have values assigned to them (via LoadFromJSON).

https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/googleapi/generator/googlediscoverytopas.pp#L303

SaveToJSON calls SavePropertyToJSON which checks IsPropertyModified:

https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/fcl-web/src/base/restbase.pp#L1310

If the property has not been modified it is skipped and not saved. But ALL of the properties are being skipped since none of them have been marked as modified (even though they were actually modified during the loading).

The reason this happens is because LoadFromJSON calls "StopRecordPropertyChanges" before loading and none of the properties get marked as "changed".

https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/fcl-web/src/base/restbase.pp#L1333

Is this the correct behavior? It seems to me the code should be:

  StartRecordPropertyChanges;
  For D in JSON Do
    LoadPropertyFromJSON(D.Key,D.Value);
  StopRecordPropertyChanges;

(or alternately get rid of the Start/Stop record property calls completely from LoadFromJSON and let the ObjectOptions control it: https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/fcl-web/src/base/restbase.pp#L559 TBaseObject.Create sets ooStartRecordingChanges by default.)

Some related misunderstandings:

1) When StopRecordPropertyChanges is called it destroys all the change records (stored in TBits) in which case SaveToJSON saves ALL the properties even if they were not loaded or modified previously.

2) Nothing in restbase.pp or TBaseObject calls MarkPropertyChanged anyway, so no properties will get marked as modified during TGoogleRestDescription.LoadFromJSON.

procedure TBaseObject.MarkPropertyChanged(AIndex: Integer);
begin
  If Assigned(FBits) then
    FBits.SetOn(GetParentPropCount+(AIndex shr IndexShift));
end;

3) Both MarkPropertyChanged (above) and IsPropertyModified (below) require the properties declarations to have indexes and TGoogleRestDescription does not have indexes on any of its properties.

function TBaseObject.IsPropertyModified(Info: PPropInfo): Boolean;
begin
  Result:=Not Assigned(FBits) or FBits.Bits[Info^.NameIndex]
end;

Is there a better way to track changed TBaseObject properties (and get rid of property index declarations)?

Lazarus knows which properties on a form have been changed from their default value and only properties which do not match the default are saved in the lfm file. Can the same thing be used with TBaseObject properties?

No, modifying means changed in user code after load.

The life cycle is one of

a/
- Load data from server. Data is 'unmodified'.
- Optionally modify properties
- If data was modified, send patch to server.

b/
- Create object
- Set properties
- Send post to server.

So the current behaviour is correct.

Michael.

That is why StopRecordPropertyChanges is called before loading.

Markpropertychanged is called in the setter of the properties generated by the code generator: Check all generated units.

TGoogleRestDescription is readonly, it does not need that mechanism.

3) Both MarkPropertyChanged (above) and IsPropertyModified (below) require the properties declarations to have indexes and TGoogleRestDescription does not have indexes on any of its properties.

See above, point 2.

Michael.

Wayne Sherman wrote

Is there a better way to track changed TBaseObject properties (and get rid of property index declarations)?

Not to my knowledge. If there had been, I would have used it. The advantage of the index as opposed to using a custom assigned ID per property is that the compiler enforces them and stores them in RTTI. thus they are a safe mechanism.

No, since you don't have a 'default' value as in the lazarus sense: In lazarus, the default is a default as declared in the class.

In rest, the default value is what you got from the server. (or empty in case you create a new object)

Your misunderstanding is that you consider any write of a property a modification, as in Lazarus.

But in rest, only writes that happen after the load from the server are modifications.

Hence the stoprecording/startrecording which seems wrong to you, but is in fact correct and as designed.

Michael.

PM Michael Van Canneyt wrote

Markpropertychanged is called in the setter of the properties generated by the code generator: Check all generated units.

Thanks for your explanations, it is getting more clear how modified property tracking is being used.

More clarification please. MarkPropertyChanged offsets the property changed index (to TBits) by the ClassParent total property count, but IsPropertyModified does not take this offset into account. Consider this example:

TBaseObject --> TGoogleBaseObject --> TSchema --> TMySchema
TBaseObjectClass = Class of TBaseObject;

If the object is TMySchema, in GetParentPropCount the TBits index gets offset by (see reference code below):

  TBaseObjectClass(TSchema).GetTotalPropCount;

If the object is TSchema, the TBits index gets offset by:

  TBaseObjectClass(TGoogleBaseObject).GetTotalPropCount;

Questions:
1) Since the ClassParent is always cast as TBaseObjectClass isn't this the same as just TBaseObjectClass(Self.ClassType)?

2) Doesn't each object already have the parent properties included in its own properties by inheritance?

3) Why does MarkPropertyChanged offset the index and IsPropertyModified does not take the offset into account?

Reference for above questions:

class function TBaseObject.GetParentPropCount: Integer;
begin
  if (ClassParent=TBaseObject) or (ClassParent=Nil) then
    Result:=0
  else
    Result:=TBaseObjectClass(ClassParent).GetTotalPropCount;
end;
procedure TBaseObject.MarkPropertyChanged(AIndex: Integer);
begin
  If Assigned(FBits) then
    FBits.SetOn(GetParentPropCount+(AIndex shr IndexShift));
end;
function TBaseObject.IsPropertyModified(Info: PPropInfo): Boolean;
begin
  Result:=Not Assigned(FBits) or FBits.Bits[Info^.NameIndex]
end;

</end reference code>

No. The method is virtual, so the actual descendant's method is called.

2) Doesn't each object already have the parent properties included in its own properties by inheritance?

Why this question ?
An object does not know the property count of the parent.

3) Why does MarkPropertyChanged offset the index and IsPropertyModified does not take the offset into account?

They should be the same. I don't remember why I created the two methods differently.

Michael.

True, but why does it need to know the property count of the parent?

  TBaseClass = class
  private
    FField1: integer;
  protected
    FPropModifiedFlags: TBits;
  published
    property Field1: integer read FField1 write FField1;
  end;

GetTypeData(TBaseClass.ClassInfo)^.PropCount = 1

  TDescendant = class(TBaseClass)
  private
    FField2: integer;
  published
    property Field2: integer read FField2 write FField2;
  end;

GetTypeData(TDescendant.ClassInfo)^.PropCount = 2

TDescendant has two properties (one inherited and one declared directly) and let's say it can record if any of the properties have been modified. So why does it have to know about the number of properties it inherited vs the properties it declares directly?

Because there may be other published properties that are not part of the REST protocol. You may for instance decide to add published properties to a base class that describe where to save the object in a local database.

Michael.

For property index specifiers, it seems that NameIndex might be the wrong index used in IsPropertyModified:

function TBaseObject.IsPropertyModified(Info: PPropInfo): Boolean;
begin
  Result:=Not Assigned(FBits) or FBits.Bits[Info^.NameIndex]
end;

and possibly the rtl docs are incorrect(?):
https://www.freepascal.org/docs-html/rtl/typinfo/tpropinfo.html Index - Index for array properties
NameIndex - Index for indexed properties << IS THIS CORRECT?

In the typinfo.pp unit, a comment indicates that NameIndex has to do
with property names (i.e. not the index specifier)
https://gitlab.com/freepascal.org/fpc/source/-/blob/main/rtl/objpas/typinfo.pp#L1689
        // Don't overwrite properties with the same name
        if PropList^[TP^.NameIndex]=nil then
          PropList^[TP^.NameIndex]:=TP;
        // Point to TP next propinfo record.
        // Located at Name[Length(Name)+1] !
        TP:=aligntoptr(PPropInfo(pointer(@TP^.Name)+PByte(@TP^.Name)^+1));
        Dec(Count);
      end;

NameIndex in Delphi typeinfo also has to do with Names of properties and Index has the information on indexed properties: https://docwiki.embarcadero.com/Libraries/Sydney/en/System.TypInfo.TPropInfo Index - The index of the property. It is used as a parameter to the GetProc and SetProc methods on indexed properties. NameIndex - The offset into the Name field where the property's name starts.

Ok, that clears up that. If I understand correctly, the behavior should be as follows:

(restbase.pp)
In TBaseObject descendants, SaveToJSON only saves object properties defined directly in the final descendant class and none of the properties of parent classes. This is implemented by only marking properties as modified that are defined directly in the final descendant class and skipping properties of all parent classes (which always appear to be unmodified, and thus not saved). This prevents properties which are not part of the REST protocol from contaminating the generated JSON. It follows that REST protocol properties should always be defined in the final TBaseObject descendant class (and not in any base or intermediate classes).

While in practice most REST classes will be the final classes, it should be possible to support inheritance in rest classes, that is why the 'ParentPropertyCount' exists and is taken into account.

Michael.

Michael wrote

>> Because there may be other published properties that are not part of the >> REST protocol. You may for instance decide to add published properties to a >> base class that describe where to save the object in a local database.

Michael wrote

While in practice most REST classes will be the final classes, it should be possible to support inheritance in rest classes, that is why the 'ParentPropertyCount' exists and is taken into account.

My summary above is incorrect. How about this?:

In TBaseObject descendants, SaveToJSON only saves object properties which have had their properties marked as modified using MarkPropertyChanged (modified flags are stored internally using TBits). Properties which have not been marked as modified will not be saved to JSON. This prevents properties which are not part of the REST protocol from contaminating the generated JSON.

Auto generated code that defines TBaseObject descendant classes declares properties with index specifiers and setter methods that call MarkPropertyChanged. When a property is changed in these auto generated classes, that property will end up in the output of SaveToJSON. Properties which are a valid part of the REST protocol, but which have not been changed are not included in the generated JSON.

Properties can be defined in a TBaseObject final descendant class or parent classes and be included in the JSON output (as long as MarkPropertyChanged has been called with the corresponding index for that property).

I see a couple of problems TBaseObject.SaveToJSON

1) TBaseObject.SaveToJSON cannot distinguish properties that are part of the REST protocol from properties that are not part of it. It only knows that properties which have been modified are part of the rest protocol, but properties which have not been modified might be part of the REST protocol or might not be. For example, a client receives a JSON object from a server (via LoadFromJSON) and wants to persist the complete JSON object to disk, database, or send it to another server. But, at present, there is no way to save all the properties that are part of the REST protocol without getting contaminated with non-REST properties.

2) Lack of control. With TBaseObject.SaveToJSON it would be nice to control what is saved (Save all REST properties, or Save modified REST properties).

The non-REST properties will never be marked as changed, so they will not be saved.

2) Lack of control. With TBaseObject.SaveToJSON it would be nice to control what is saved (Save all REST properties, or Save modified REST properties).

It should be easy to add a flag for that.

Michael.

LoadFromJSON calls StartRecordPropertyChanges and
StopRecordPropertyChanges. Both of these destroy the change records. At present, there is no opportunity to save modified records after loading. If that was fixed I think it would be a step in the right direction.

Ref:
https://gitlab.com/freepascal.org/fpc/source/-/blob/main/packages/fcl-web/src/base/restbase.pp#L1148

Taking a step further, it would be advantageous if TBaseObject and descendants know specifically which of their properties are REST protocol properties independent of whether they have been modified or not. For example, if I want to dump the JSON for an empty object to see the fields, I would like to do this:

KeepNote := TNote.Create(nil);  //Google Keep Note
KeepNote.SaveAsJSON(saveAllProperties);

{
"name": "",
"createTime": "",
"updateTime": "",
"trashTime": "",
"trashed": false,
"attachments": [
{ }
],
"permissions": [
{ }
],
"title": "",
"body": { }
}

It seems to me we're talking cross-purpose. From my point of view, the code works as designed. It has been in use in production since many years (in fact, since it was committed to FPC).

Can you please make a small example that demonstrates the problem you are experiencing, so I can look at it ?

Michael.

Ok, I will see if I can create a demo. I need clarification about the auto generated class index specifiers. Do they always start at 0 in each descendant class, or are they unique across all descendant classes?

TBaseObject --> TChild --> TGrandChild

TChild = class(TBaseObject)
...
  published
    property Field1: string index 0...
    property Field2: string index 8...
  end;
TGrandChild = class(TChild)
...
  published
    property Field3: string index 16...  // does this continue at 16
or start at 0 again?
    property Field4: string index 24...
  end;

It appears the index specifiers restart at 0 in each descendant class. Which is why this code works:

procedure TBaseObject.MarkPropertyChanged(AIndex: Integer);
begin
  If Assigned(FBits) then
    FBits.SetOn(GetParentPropCount+(AIndex shr IndexShift));
end;
function TBaseObject.IsPropertyModified(Info: PPropInfo): Boolean;
begin
  Result:=Not Assigned(FBits) or FBits.Bits[Info^.NameIndex]
end;

The name list (from which NameIndex gets its value) includes the published property names of all the parents plus the names in the current class, so for a REST property in a descendant class with Index specifiers starting at 0:
GetParentPropCount+(IndexSpecifier shr IndexShift) = NameIndex

Yes. The reason is the code generator:

Ideally, every property simply has a unique index specifier.

But when generating classes, the code generator has no way of knowing how many properties exist in parent classes, so it must start at 0.

(
one could imagine helping the code generator by specifying a start point per class:
1000
2000
3000
etc.
)

Yes.

This was long ago, but I seem to remember that I changed that code in production, so it would use the same mechanism as MarkPropertyChanged, because there were published properties not part of the REST scheme, and in that case, the equation does not hold true (becasue nameindex includes non-REST properties). That change does not seem to have made it in FPC.

Michael.

If non-REST properties are always not published the codegen could generate unique index specifiers. Also, if 0 is not used as an index specifier, then any non-zero index specifier could be used to indicate a REST property. (* but I have more to say about this below)

A REST class should know how to generate a complete and correct JSON representation, so there must be a way to distinguish REST properties from non-REST. Which properties have been modified is not sufficient. I thought any property with an index specifier could be considered REST, but the first property has an index specifier of 0 which matches other properties with no index specifier. One of the extra bit flags in the index specifier could be used to indicate a REST property since bits 0-2 seem to be reserved for future use. But since the mechanism for persisting objects is based on typeinfo and published properties, I think the rule should be that ALL REST properties are published and ALL non-REST properties are NOT published. Why would you need to publish any non-REST properties?

AM Wayne Sherman wrote

A REST class should know how to generate a complete and correct JSON representation, so there must be a way to distinguish REST properties from non-REST. Which properties have been modified is not sufficient.

Rephrasing that last sentence: Tracking "which properties have been modified" is sufficient for a limited use case. But with a way to distinguish REST properties and some other relatively small adjustments, TBaseClass could be more generally useful for many other use cases.