proposed unitTests

[dberry-rcs]

@dj-nitehawk Here is how I propose to do the unitTests. I had originally started here, but MongoDB.Entities does not support dynamic lookup of Cache data. Cache is really a static thing and does not support Interfaces or abstract classes. I tried getting the tests to work several other ways, but c# does not support the wildcard generics.

I submit this PR to show how I would like the tests to work. Before I go through the process of having Cache walk the inheritence of an Object to find the IEntity that might exist, I wanted to make sure you are okay with this. I think this would make MongoDB.Entities more powerful by supporting interfaces and abstractions.

This PR builds, but fails quickly as Cache does not know what to do with the abstract class which does not contain an Id. The implementing class does contain an Id, and we could make this work. This may end up being a big design change and want to get your thoughts on how to proceed.

[dj-nitehawk]

testing stratergy looks alright to my eyes. quick question though... would the "cache walking" need to happen repetitively on a per operation basis incurring a performance hit? if so, can we cache the result of walking and avoid repetitive walks for a given type maybe? also want to clarify, this won't be a breaking change to the existing public api right?

[dberry-rcs]

We will still need to do the walk each time, but we should only create the cached data once for the class and the walk should see that the class has cache data. E.G. PLACE -> PlaceEntity would create all the cache data for PlaceEntity the first time. Subsequent walks would hit PlaceEntity, and find the cached data.

Should not be a breaking change. The walk will not occur for any existing classes. If we get to the point of where we throw the exception today for no ID, we will do the inheritence walk. So only if the existing Cache is going to throw an exception do we do a final inheritance check.

[dj-nitehawk]

alright then... carry on... let's see how it goes...

[dberry-rcs]

Quick Update. Still trying to make sure there are no breaking changes, but looks like Cache will need to become an extension method with the following signature: internal static Cache<T> Cache<T>(this T entity) where T : IEntity

the class Cache will lose its static modifier. Most likely going to need a dictionary of <string, Cache> to act as the cache.

This will have a ripple effect on the rest of the library as well. Some call that were basically static will get changes to extension methods to be dynamic.

[dberry-rcs]

Here is a look at where I am headed. I did not realize that MongoDB itself does not support inheritance either, other than letting you jam related classes in the same collection with discriminators. Makes it harder but not impossible to do what I want.

Was wondering if you could skip the tests and run the benchmarks to see if this is performant enough. The signatures are: internal static Cache Get(T entity) internal static Cache Get(Type type)

Not sure this is going to work, but my tests are taking longer to fail now, and it is a small enough test set to see if I can get this working or not.

[dberry-rcs]

Back to just copying tests and changing the class type. Tried using reflection, but once you go reflection, everything is reflection. I'll keep adding tests in this way. Trying to have all tests work with another Id type.

[dj-nitehawk]

alrighty... duplicate tests it is then...

[dberry-rcs]

@dj-nitehawk Can you check this commit? https://github.com/dj-nitehawk/MongoDB.Entities/pull/208/commits/a89b36b145dcaef707bd40b5b5edba6b7a0310c0. There was a race condition between two tests. I stopped one test from using Image and the other test started passing. Wondering if there is an issue with NextSequentialNumberAsync.

[dberry-rcs]

@dj-nitehawk So it works, sort of. For the relationships to work, the serialization code needs to be added to the AsBsonIdAttribute for each id type someone wants to use. Do you have any ideas on how to make the relationships work without [AsBsonId]?

Also, You can't pass an array of ids to the library. new[] { b.ID, b1,ID } get treated as object? and not object?[] for the struct ID types of Int64, Guid, Uuid7, etc. The array has to be fully qualified as new object?[] {b.ID, b1,ID} . Not the worst thing in the world, but not obvious to the untrained observer.

[dj-nitehawk]

Can you check this commit? a89b36b. There was a race condition between two tests. I stopped one test from using Image and the other test started passing. Wondering if there is an issue with NextSequentialNumberAsync.

sorry been very busy. do i still need to look in to the race condition thing or has it been sorted out?

[dberry-rcs]

Can you check this commit? a89b36b. There was a race condition between two tests. I stopped one test from using Image and the other test started passing. Wondering if there is an issue with NextSequentialNumberAsync.

sorry been very busy. do i still need to look in to the race condition thing or has it been sorted out?

It is still there.

[dj-nitehawk]

Can you check this commit? a89b36b. There was a race condition between two tests. I stopped one test from using Image and the other test started passing. Wondering if there is an issue with NextSequentialNumberAsync.

sorry been very busy. do i still need to look in to the race condition thing or has it been sorted out?

It is still there.

all the tests were passing on my machine. i changed it to ParallelForEachAsync() anyway. can you pls check again if it made any difference on your machine.

[dberry-rcs]

Can you check this commit? a89b36b. There was a race condition between two tests. I stopped one test from using Image and the other test started passing. Wondering if there is an issue with NextSequentialNumberAsync.

sorry been very busy. do i still need to look in to the race condition thing or has it been sorted out?

It is still there.

all the tests were passing on my machine. i changed it to ParallelForEachAsync() anyway. can you pls check again if it made any difference on your machine.

I reverted the code to the original. If I use an Image in this test, deleting_entity_deletes_all_chunks() fails as a result.

If I run deleting_entity_deletes_all_chunks() by itself, it works. There is a race condition or data mangling between deleting_entity_deletes_all_chunks() and next_sequential_number_for_entities_multidb.

[dj-nitehawk]

i can't get it to fail locally. but it does fail in ci/cd. i've merged this to dev branch. let's continue working off of that otherwise it's going to get messy keeping the two in sync without conflicts.

[dj-nitehawk]

just ran the benchmarks against the dev branch and it seems quite good. no significant perf drop from the dynamic ID lookup stuff.

so.... i feel like we should merge dev on to master and put out a beta release. could you pls have a play with dev and let me know if that sounds like a plan...

oh and Entity.ID is now non-nullable.

[dj-nitehawk]

also... when we go public with this, we need to remember to make a note in the docs to mention that value type Id properties need to be nullable if the user expect the library to automatically generate Ids.

or we need to figure out a way to modify this to cater for that:

https://github.com/dj-nitehawk/MongoDB.Entities/blob/a25234c9122f5b4e4d200354b0e4e4912f08e38f/MongoDB.Entities/DB/DB.Save.cs#L279-L291

[dberry-rcs]

just ran the benchmarks against the dev branch and it seems quite good. no significant perf drop from the dynamic ID lookup stuff.

so.... i feel like we should merge dev on to master and put out a beta release. could you pls have a play with dev and let me know if that sounds like a plan...

oh and Entity.ID is now non-nullable.

I tried to set the non-string IDs to not null, but cannot use "= null!;" as you did. Types like ObjectId and Int64 throw an error on this. Are you okay with adding a method like bool IsSetID() to IEntity, so that each ID type can check its own default value and let you know if it is the default value for this ID Type?