iluvadev
commented
1 year ago 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:
https://github.com/iluvadev/PocketBaseClient/blob/a7a37faf6e333d612d8894458541e4b162fac869/src/PocketBaseClient/Orm/CollectionBase%5BT%5D.Repository.cs#L134-L201
_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:
- Returns all new items in-memory
- Invalidates all downloaded items in cache: they do not have the latest info and maybe are not in server
- Downloads all objects from server, updating caches
- 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
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
```JSON [ { "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
```JSON { "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
```JSON { "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
```C# 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,
Test1collection has a relation field ofTest2collection (single records, not multipe records) namedField.NotWorkingProperlymethod inReproduction coderetrieves records of Test1 collection at first. At that time, it does contain a list of pseudo objects inFieldproperty, but does not contain a value ofHandJproperty in each pseudo objects ofFieldproperty. So inside of the foreach statement, a value ofHandJproperty are retrieved at the time of these properties being accessed by getter of it. Getter of ItemBase callsFillFromPbAsyncat the end.https://github.com/iluvadev/PocketBaseClient/blob/a7a37faf6e333d612d8894458541e4b162fac869/src/PocketBaseClient/Orm/CollectionBase%5BT%5D.Repository.cs#L110-L121
It updates items correctly, but it does not update
_PocketBaseItemsCountproperty ofCollectionBase<T>class.Let's go back to
Reproduction code. After that, It callsWorkingProperlymethod.WorkingProperlymethod accessesHandJproperty from this time,FirstOrDefault. At the time of callingFirstOrDefault, Test2Collection callsGetItemsto get enumerator, and it callsGetItemsInternalto retrieves records at the end.https://github.com/iluvadev/PocketBaseClient/blob/a7a37faf6e333d612d8894458541e4b162fac869/src/PocketBaseClient/Orm/CollectionBase%5BT%5D.Repository.cs#L134-L201
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 ofidproperty.https://github.com/iluvadev/PocketBaseClient/blob/a7a37faf6e333d612d8894458541e4b162fac869/src/PocketBaseClient/Orm/ItemBase.cs#L26-L36
Collection.ChangeIdInCachecallsUpdateWithat the end.UpdateWithofTest2class, which is generated by pbcodegen is as follows.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.Hhas its value, butitem.Jdoes not have its value. It's becauseJproperty is deserialized afteridproperty. Let me explain more.Response of PocketBase API is as follows.
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 thatJproperty is deserialized right afterIdproperty is being deserialized. Due to this,item.Hhas its value, butitem.Jdoes not have its value at the time ofUpdateWithbeing 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
Idbeing deserialized, not all of the properties being deserialized. So I added constructor for each items by using[JsonConstructor]to make sure thatUpdateWithis being called after all properties being set. And also, I added a reference equality check inUpdateWithto avoid miss update. (If reference equals,RemoveAllbreaks impl)