Fix invalid json deserialization

[mtaku3]

About the issue

It's a bit difficult to explain in text. So I made a minimal setup of reproduction and explain what happens behind the code step by step.

Schema of collection

[
    {
        "id": "and4p1eq95kv695",
        "name": "test1",
        "type": "base",
        "system": false,
        "schema": [
            {
                "id": "txkcgoew",
                "name": "field",
                "type": "relation",
                "system": false,
                "required": true,
                "unique": false,
                "options": {
                    "collectionId": "2lh048rn4ur83kl",
                    "cascadeDelete": false,
                    "maxSelect": 1,
                    "displayFields": []
                }
            }
        ],
        "listRule": "",
        "viewRule": "",
        "createRule": null,
        "updateRule": null,
        "deleteRule": null,
        "options": {}
    },
    {
        "id": "2lh048rn4ur83kl",
        "name": "test2",
        "type": "base",
        "system": false,
        "schema": [
            {
                "id": "p3974g5d",
                "name": "h",
                "type": "text",
                "system": false,
                "required": true,
                "unique": false,
                "options": {
                    "min": null,
                    "max": null,
                    "pattern": ""
                }
            },
            {
                "id": "fuqk8cf5",
                "name": "j",
                "type": "text",
                "system": false,
                "required": true,
                "unique": false,
                "options": {
                    "min": null,
                    "max": null,
                    "pattern": ""
                }
            }
        ],
        "listRule": "",
        "viewRule": "",
        "createRule": null,
        "updateRule": null,
        "deleteRule": null,
        "options": {}
    }
]

Records of Test1 collection

{
  "page": 1,
  "perPage": 30,
  "totalItems": 3,
  "totalPages": 1,
  "items": [
    {
      "collectionId": "and4p1eq95kv695",
      "collectionName": "test1",
      "created": "2023-02-04 03:50:15.738Z",
      "field": "h4rvxzuxuf4wom0",
      "id": "c4qh01f9t23cws4",
      "updated": "2023-02-04 09:14:32.097Z"
    },
    {
      "collectionId": "and4p1eq95kv695",
      "collectionName": "test1",
      "created": "2023-02-04 03:50:40.302Z",
      "field": "ugoizglgmoyw2ef",
      "id": "uevwfoyey6lpr5b",
      "updated": "2023-02-04 09:14:28.275Z"
    },
    {
      "collectionId": "and4p1eq95kv695",
      "collectionName": "test1",
      "created": "2023-02-04 03:50:46.153Z",
      "field": "96wi435icppx2rb",
      "id": "bdgpqv3iwvz3ke4",
      "updated": "2023-02-04 09:14:23.897Z"
    }
  ]
}

Records of Test2 collection

{
  "page": 1,
  "perPage": 30,
  "totalItems": 3,
  "totalPages": 1,
  "items": [
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:52.427Z",
      "h": "1",
      "id": "h4rvxzuxuf4wom0",
      "j": "1",
      "updated": "2023-02-04 11:47:28.201Z"
    },
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:56.010Z",
      "h": "2",
      "id": "ugoizglgmoyw2ef",
      "j": "2",
      "updated": "2023-02-04 11:47:25.211Z"
    },
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:58.341Z",
      "h": "3",
      "id": "96wi435icppx2rb",
      "j": "3",
      "updated": "2023-02-04 11:47:22.547Z"
    }
  ]
}

Reproduction code

internal class Program
{
    private static void Main(string[] args)
    {
        var pbClient = new AcmeApplication();
        NotWorkingProperly(pbClient);
    }

    // Expected output and actual output is the same as follows
    //
    // H    J
    // 1    1
    // 2    2
    // 3    3
    private static void WorkingProperly(AcmeApplication pbClient)
    {
        var cnt = pbClient.Data.Test2Collection.Count();
        var idx = 0;
        Console.WriteLine("H\tJ");
        pbClient.Data.Test2Collection.FirstOrDefault(x =>
        {
            Console.WriteLine($"{x.H}\t{x.J}");
            return ++idx == cnt;
        });
    }

    // Expected output is
    //
    // H    J
    // 1    1
    // 2    2
    // 3    3
    //
    // H    J
    // 1    1
    // 2    2
    // 3    3
    //
    // Actual output is
    //
    // H    J
    // 1    1
    // 2    2
    // 3    3
    //
    // H    J
    // 1    
    // 2    
    // 3    
    private static void NotWorkingProperly(AcmeApplication pbClient)
    {
        Console.WriteLine("H\tJ");
        foreach (var test1 in pbClient.Data.Test1Collection)
        {
            var test2 = test1.Field;
            Console.WriteLine($"{test2?.H}\t{test2?.J}");
        }

        Console.Write("\n");

        WorkingProperly(pbClient);
    }
}

Expected outputs and actual outputs are written in Reproduction code.

PocketBaseClient retrieves records of a collection without expanding relation fields. So it creates pseudo objects, which only contains id in it.
In this example, Test1 collection has a relation field of Test2 collection (single records, not multipe records) named Field.
NotWorkingProperly method in Reproduction code retrieves records of Test1 collection at first. At that time, it does contain a list of pseudo objects in Field property, but does not contain a value of H and J property in each pseudo objects of Field property. So inside of the foreach statement, a value of H and J property are retrieved at the time of these properties being accessed by getter of it.
Getter of ItemBase calls FillFromPbAsync at the end.

PocketBaseClient/src/PocketBaseClient/Orm/CollectionBase[T].Repository.cs

Lines 110 to 121 in a7a37fa

	private async Task<bool> FillFromPbAsync(T item)
	{
	if (item.Id == null) return false;
	var loadedItem = await PocketBase.HttpGetAsync<T>(UrlRecord(item));
	if (loadedItem == null) return false;
	loadedItem.Metadata_.SetLoaded();
	item.UpdateWith(loadedItem);
	item.Metadata_.SetLoaded();
	return true;
	}

It updates items correctly, but it does not update _PocketBaseItemsCount property of CollectionBase<T> class.

Let's go back to Reproduction code. After that, It calls WorkingProperly method.
WorkingProperly method accesses H and J property from this time, FirstOrDefault.
At the time of calling FirstOrDefault, Test2Collection calls GetItems to get enumerator, and it calls GetItemsInternal to retrieves records at the end.

PocketBaseClient/src/PocketBaseClient/Orm/CollectionBase[T].Repository.cs

Lines 134 to 201 in a7a37fa

	private IEnumerable<T> GetItemsInternal(bool reload = false, GetItemsFilter include = GetItemsFilter.Load | GetItemsFilter.New)
	{
	//No marcar com a necessita recarregar si té canvis locals! O gestionar-ho bé!!
	var allCachedItems = Cache.AllItems.ToList();
	// Count not new Items to compare with _PocketBaseCount
	if (!reload && allCachedItems.Count(i => !i.Metadata_.IsNew) == _PocketBaseItemsCount)
	{
	// Return cached items
	foreach (var item in allCachedItems)
	{
	// Check if item must be returned
	if (item.Metadata_.MatchFilter(include))
	yield return item;
	}
	}
	else
	{
	// Return cached new items if must be returned
	if ((include & GetItemsFilter.New) == GetItemsFilter.New)
	foreach (var item in allCachedItems.Where(i => i.Metadata_.SyncStatus == ItemSyncStatuses.ToBeCreated))
	yield return item;
	// Clean cached items and return items from PocketBase
	// Set all cached loaded items as NeedToBeLoaded
	var idsToTrash = new List<string>();
	foreach (var notNewItem in allCachedItems.Where(i => i.Metadata_.SyncStatus != ItemSyncStatuses.ToBeCreated))
	{
	notNewItem.Metadata_.SetNeedBeLoaded();
	idsToTrash.Add(notNewItem.Id!);
	}
	// Get Items from PocketBase
	int loadedItems = 0;
	int currentPage = 1;
	while (_PocketBaseItemsCount == null || loadedItems < _PocketBaseItemsCount)
	{
	var page = GetPageFromPbAsync(currentPage).Result;
	if (page != null)
	{
	currentPage++;
	_PocketBaseItemsCount = page.TotalItems;
	var pageItems = page.Items ?? Enumerable.Empty<T>();
	loadedItems += pageItems.Count();
	foreach (var item in pageItems)
	idsToTrash.Remove(item.Id!);
	foreach (var item in pageItems)
	// Check if item must be returned
	if (item.Metadata_.MatchFilter(include))
	yield return item;
	}
	}
	// Mark as Trash all not downloaded
	foreach (var idToTrash in idsToTrash)
	{
	var itemToTrash = Cache.Get(idToTrash);
	if (itemToTrash != null)
	itemToTrash.Metadata_.IsTrash = true;
	}
	Cache.RemoveTrash();
	}
	}

At this time, if statement of line 141 is false since _PocketBaseItemsCount is still null as I said earlier.
So it goes to line 173 and calls GetPageFromPbAsync.
After that, records are not manually added to its collection. It's by design and it seems to be automatically added by internal setter of id property.

PocketBaseClient/src/PocketBaseClient/Orm/ItemBase.cs

Lines 26 to 36 in a7a37fa

	public new string? Id
	{
	get => _Id ?? "";
	internal set
	{
	var oldValue = _Id;
	_Id = value;
	if (oldValue != null && value != oldValue)
	Collection.ChangeIdInCache(oldValue, this);
	}
	}

Collection.ChangeIdInCache calls UpdateWith at the end. UpdateWith of Test2 class, which is generated by pbcodegen is as follows.

        public override void UpdateWith(ItemBase itemBase)
        {
            base.UpdateWith(itemBase);

            if (itemBase is Test2 item)
            {
                H = item.H;
                J = item.J;
            }
        }

As you can see, it does not update the reference of records, but it does overwrite each properties of current records in the collection.
But at this time, item.H has its value, but item.J does not have its value.
It's because J property is deserialized after id property. Let me explain more.

Response of PocketBase API is as follows.

{
  "page": 1,
  "perPage": 30,
  "totalItems": 3,
  "totalPages": 1,
  "items": [
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:52.427Z",
      "h": "1",
      "id": "h4rvxzuxuf4wom0",
      "j": "1",
      "updated": "2023-02-04 11:47:28.201Z"
    },
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:56.010Z",
      "h": "2",
      "id": "ugoizglgmoyw2ef",
      "j": "2",
      "updated": "2023-02-04 11:47:25.211Z"
    },
    {
      "collectionId": "2lh048rn4ur83kl",
      "collectionName": "test2",
      "created": "2023-02-04 03:49:58.341Z",
      "h": "3",
      "id": "96wi435icppx2rb",
      "j": "3",
      "updated": "2023-02-04 11:47:22.547Z"
    }
  ]
}

As you can see, each properties are lexical ordered. (collectionId -> collectionName -> created -> h -> id -> j -> updated)
System.Text.Json's deserializer goes through the response from top to bottom. So that J property is deserialized right after Id property is being deserialized.
Due to this, item.H has its value, but item.J does not have its value at the time of UpdateWith being called.

This is the entire picture of the issue.
I'm so sorry for long explation and I couldn't explain it well. Feel free to ask me if you don't understand.

How I fixed it

The cause of the issue is that collection updates is called at the time of Id being deserialized, not all of the properties being deserialized.
So I added constructor for each items by using [JsonConstructor] to make sure that UpdateWith is being called after all properties being set.
And also, I added a reference equality check in UpdateWith to avoid miss update. (If reference equals, RemoveAll breaks impl)

[iluvadev]

I understand it, thanks a lot.
Your solution with constructors is the best: a mechanism to assign all properties at once, without unnecessary operations complicating it and adding error cases.

Seeing your example I see that maybe there are design errors. I think the GetItemsInternal function needs a rethink
Let me explain some things of the GetItemsInternal function:

PocketBaseClient/src/PocketBaseClient/Orm/CollectionBase[T].Repository.cs

Lines 134 to 201 in a7a37fa

	private IEnumerable<T> GetItemsInternal(bool reload = false, GetItemsFilter include = GetItemsFilter.Load | GetItemsFilter.New)
	{
	//No marcar com a necessita recarregar si té canvis locals! O gestionar-ho bé!!
	var allCachedItems = Cache.AllItems.ToList();
	// Count not new Items to compare with _PocketBaseCount
	if (!reload && allCachedItems.Count(i => !i.Metadata_.IsNew) == _PocketBaseItemsCount)
	{
	// Return cached items
	foreach (var item in allCachedItems)
	{
	// Check if item must be returned
	if (item.Metadata_.MatchFilter(include))
	yield return item;
	}
	}
	else
	{
	// Return cached new items if must be returned
	if ((include & GetItemsFilter.New) == GetItemsFilter.New)
	foreach (var item in allCachedItems.Where(i => i.Metadata_.SyncStatus == ItemSyncStatuses.ToBeCreated))
	yield return item;
	// Clean cached items and return items from PocketBase
	// Set all cached loaded items as NeedToBeLoaded
	var idsToTrash = new List<string>();
	foreach (var notNewItem in allCachedItems.Where(i => i.Metadata_.SyncStatus != ItemSyncStatuses.ToBeCreated))
	{
	notNewItem.Metadata_.SetNeedBeLoaded();
	idsToTrash.Add(notNewItem.Id!);
	}
	// Get Items from PocketBase
	int loadedItems = 0;
	int currentPage = 1;
	while (_PocketBaseItemsCount == null || loadedItems < _PocketBaseItemsCount)
	{
	var page = GetPageFromPbAsync(currentPage).Result;
	if (page != null)
	{
	currentPage++;
	_PocketBaseItemsCount = page.TotalItems;
	var pageItems = page.Items ?? Enumerable.Empty<T>();
	loadedItems += pageItems.Count();
	foreach (var item in pageItems)
	idsToTrash.Remove(item.Id!);
	foreach (var item in pageItems)
	// Check if item must be returned
	if (item.Metadata_.MatchFilter(include))
	yield return item;
	}
	}
	// Mark as Trash all not downloaded
	foreach (var idToTrash in idsToTrash)
	{
	var itemToTrash = Cache.Get(idToTrash);
	if (itemToTrash != null)
	itemToTrash.Metadata_.IsTrash = true;
	}
	Cache.RemoveTrash();
	}
	}

_PocketBaseItemsCount is used to store the number of items in PocketBase server collection. It is set when is known, when PocketBase says to us the real number of items in the collection, then this value is cached in _PocketBaseItemsCount.
In line 141 check if all items are cached and there is no need to get items from PocketBase.
In the case of having to talk to the server to get the items:

1. Returns all new items in-memory
2. Invalidates all downloaded items in cache: they do not have the latest info and maybe are not in server
3. Downloads all objects from server, updating caches
4. Removes old items in cache that are not in server

I think that the case of having to talk to the server to get the items should be rethought or redesigned: Step 2 is confusing and maybe it's not necessary.

But this is not the cause of the problem you describe, although being a complex process, it affects.

About your solution: PERFECT!
There's a lot to consider, and it's my first time developing an ORM

[iluvadev]

Good!
You make me think about #30
I'm also considering to put a ReferenceEquals check in UpdateWith from ItemBase

[iluvadev]

I found an issue:
(Relaetd also with #30): In GetParameterNameForConstructor, the generated parameter name fulfills the C# naming conventions, but what if the name matches a reserved word?
Example:
I have a field with name long: PropertyName Long, AttributeName _Long, parameterName in Constructor long (compilation error)
I propose a simple solution: Start the name with _ -> _long (it's ugly and I don't quite like it)

And, what happens with fields with multiple values? I do not test it yet: the constructor spects a list of objects, I don't know if json parser deserializes correctly. I will test it.

I think your solution is very good

[iluvadev]

I found an issue: (Relaetd also with #30): In GetParameterNameForConstructor, the generated parameter name fulfills the C# naming conventions, but what if the name matches a reserved word? Example: I have a field with name long: PropertyName Long, AttributeName _Long, parameterName in Constructor long (compilation error) I propose a simple solution: Start the name with _ -> _long (it's ugly and I don't quite like it)

My proposal is wrong: I didn't take into account that the name of the constructor parameters must match the names of the properties (case insensitive). Adding an @ to the name of every parameter solves the problem: @long

[iluvadev]

I added some changes:

• In the constructors, I added always the parameters id, created and updated, and the call to parent constructor
• The constructor parameter names are prepended with @
• The ItemBase.Id setter manages if the item is new and it must to be registered in the Collection or it has an old Id and it must to be changed in the memory Collection
• UpdateWith does not update the item if it is the same object (checks ReferenceEquals)
• Fix some serialization errors when PocketBase returns "" instead of null in json responses

And relative to my comment:

And, what happens with fields with multiple values? I do not test it yet: the constructor spects a list of objects, I don't know if json parser deserializes correctly. I will test it.

It works! Parser uses the defined Converters in the Property decorators: Perfect!

Again: Thanks!! :)