Cleanup data access

[ddaspit]

• remove unnecessary Mongo attributes
• fix various coding convention inconsistencies
• reimplement upsert support
• cleanup integration tests

---

This change is 

[Enkidu93]

Looks good to me. I apologize for the inconsistencies. I did know the camelCase and CapitalFunction style rules, but apparently, I slipped into something more Pythonic which is more natural to me (I only used C/C++ and Python at my previous job so my only C# has been academic not professional). As for some of the others like not declaring multiple variables of the same type at once or preferring the var varname = new VarType() to VarType varname = new() were not familiar to me. Do you have a style guide you try to keep to or some source I could read to avoid this in the future? (I realize that these are simple, find-and-replace fixes, but still tedious and time-consuming).

Reviewable status: 0 of 29 files reviewed, all discussions resolved

[Enkidu93]

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

[johnml1135]

src/Serval.DataFiles/Models/DataFile.cs line 6 at r1 (raw file):

{
    [BsonId]
    [BsonRepresentation(BsonType.ObjectId)]

I am concerned about this - were all E2E tests run? There were a number of string to ObjectID conversions that were not working - they may all be resolved by using LINQ.V2, but I want to be sure.

[johnml1135]

Upsert was intentionally removed and shouldn't be needed. Let's discuss the issues from #36 and we should be able to agree to the path forward. It is tied to RWLock and to where a new ID is created for engines and files.

[johnml1135]

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 144 at r1 (raw file):

    {
        Engine engine = Map(engineConfig);
        engine.Id = idGenerator.GenerateId();

Here is where the ID is generated - so upsert is not needed.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 155 at r1 (raw file):

            if (rpcEx.StatusCode == Grpc.Core.StatusCode.InvalidArgument)
                return UnprocessableEntity(rpcEx.Message);
            throw;

What is thrown here? Is the RpcException thrown? Can be it be made more explicit (or is it already and I just don't know the conventions)?

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 419 at r1 (raw file):

    )
    {
        if (!(await AuthorizeAsync(id, cancellationToken)).IsSuccess(out ActionResult? errorResult))

Why was this removed? Is it redundant to AuthoriseIsOwnerAsync?

[ddaspit]

Upsert is a fundamental operation of a Document DB. We don't need to use it for distributed locks, but I don't want to remove it from a general-purpose Document DB abstraction library like SIL.DataAccess.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @johnml1135)

---

src/Serval.DataFiles/Models/DataFile.cs line 6 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I am concerned about this - were all E2E tests run? There were a number of string to ObjectID conversions that were not working - they may all be resolved by using LINQ.V2, but I want to be sure.

The BsonId attribute is automatically set by the driver for properties named Id. The BsonRepresentation(BsonType.ObjectId) attribute is set using the convention StringIdStoredAsObjectIdConvention. The convention is registered in the IServiceCollectionExtensions.AddMongoDataAccess method in the SIL.DataAccess assembly.

I ran the integration tests, and they use Mongo. What is the easiest way to run the E2E tests?

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 144 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Here is where the ID is generated - so upsert is not needed.

I'm not sure I understand what this has to do with upserts. Can you clarify more?

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 155 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

What is thrown here? Is the RpcException thrown? Can be it be made more explicit (or is it already and I just don't know the conventions)?

This is how you properly rethrow an exception in C#.

---

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 419 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Why was this removed? Is it redundant to AuthoriseIsOwnerAsync?

Yes, it is redundant.

[johnml1135]

src/Serval.Translation/Services/EngineService.cs line 288 at r1 (raw file):

                {
                    return c.Id == corpusId;
                })

Here and above we assume that en engine's Corpora can never be null. Are we always certain? how exactly do we know? Is it enforced through the data model in Engine.cs by definition?

[johnml1135]

src/SIL.DataAccess/MemoryRepository.cs line 119 at r1 (raw file):

    {
        entity.Revision = 1;
        if (string.IsNullOrEmpty(entity.Id))

These are not needed as the ID is always populated.

[johnml1135]

So, you are in agreement that we won't be using it for anything right now, but that the library should have it for completeness. I have a hard time arguing against that.

[johnml1135]

src/Serval.DataFiles/Models/DataFile.cs line 6 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

The BsonId attribute is automatically set by the driver for properties named Id. The BsonRepresentation(BsonType.ObjectId) attribute is set using the convention StringIdStoredAsObjectIdConvention. The convention is registered in the IServiceCollectionExtensions.AddMongoDataAccess method in the SIL.DataAccess assembly.

I ran the integration tests, and they use Mongo. What is the easiest way to run the E2E tests?

docker compose - I can run them on your branch.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 144 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

I'm not sure I understand what this has to do with upserts. Can you clarify more?

Upserts are ok - I agree.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 155 at r1 (raw file):

Previously, ddaspit (Damien Daspit) wrote…

This is how you properly rethrow an exception in C#.

I learn something new every day.

[johnml1135]

src/Serval.Translation/Controllers/TranslationEnginesController.cs line 155 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

I learn something new every day.

Here is documentation supporting your comment: https://stackoverflow.com/questions/881473/why-catch-and-rethrow-an-exception-in-c

[ddaspit]

@johnml1135 Yeah, that is a good summary of what I was saying.

@Enkidu93 No need to apologize. I never gave clear direction on coding conventions. I am a bit of a stickler when it comes to them. I think they greatly increase readability. Here is a good overview of naming conventions. Here is a good overview of coding conventions. If you want to get in to even more detail, check out the Framework design guidelines.

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @johnml1135)

[johnml1135]

Updated the Readme with your links.

[johnml1135]

src/Serval.DataFiles/Models/DataFile.cs line 6 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

docker compose - I can run them on your branch.

I ran them and came across no issues - especially with cancelling which was giving issues before. I am ok removing them, as it is explicit enough.

[johnml1135]

Reviewed 29 of 29 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[johnml1135]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[johnml1135]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ddaspit)

[ddaspit]

Dismissed @johnml1135 from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ddaspit)

---

src/Serval.Translation/Services/EngineService.cs line 288 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

Here and above we assume that en engine's Corpora can never be null. Are we always certain? how exactly do we know? Is it enforced through the data model in Engine.cs by definition?

There was a bug where the Corpora property could be null. I fixed that bug in this PR. From now on, it should always not be null. Unfortunately, in .NET 6, we cannot enforce this in the model definition. We can declare that the Corpora property should never be null, but we can't enforce it. Once we move to .NET 8, we can enforce it. We could also enforce it in .NET 7, but the odd number .NET releases are not LTS releases.

---

src/SIL.DataAccess/MemoryRepository.cs line 119 at r1 (raw file):

Previously, johnml1135 (John Lambert) wrote…

These are not needed as the ID is always populated.

We always populate the ID, but if we didn't, Mongo would automatically populate the ID. This makes the memory-based repository consistent with the Mongo repository.