Make Sense.PartOfSpeech an object instead of a string #1350

rmunn · 2025-01-07T22:32:44Z

Backend and frontend changes appear to be working; here's the frontend viewer showing the Sena-3 test data:

And here's the sense display, showing that all the parts of speech are being looked up properly:

There's almost certainly more work to be done, but this seems to be a good start.

rmunn · 2025-01-07T22:33:47Z

Also getting some errors in FwLiteProjectSync.Tests.SyncTests.PartsOfSpeechSyncInEntries such as "One or more errors occurred. (The instance of entity type 'PartOfSpeech' cannot be tracked because another instance with the key value '{Id: a8e41fd3-e343-4c7c-aa05-01ea3dd5cfb5}' is already being tracked."

Don't have time to track those down right now, but recording the errors here so that I remember to track them down later.

github-actions · 2025-01-07T22:35:16Z

C# Unit Tests

104 tests 104 ✅ 5s ⏱️
16 suites 0 💤
1 files 0 ❌

Results for commit 1e14f4c.

♻️ This comment has been updated with latest results.

rmunn · 2025-01-07T23:21:52Z

One place this is happening is in BulkCreateEntries during ImportProject: if more than one sense had the same part of speech (very likely), then the code is currently trying to create those as separate DB entries rather than as references to the same one. I need to change how the DB handles PartOfSpeech to be a relation (looked up by PartOfSpeechId) here.

rmunn · 2025-01-08T00:16:54Z

@hahn-kev - I've gotten most of this working, but I could use some help with some of the remaining test failures. I know there's code somewhere that's creating duplicate parts of speech in the database instead of simply making references, but I'm not sure exactly where it is. There's about one level of abstraction more than I'm managing to fit in my head right now, and I suspect that extra level of abstraction is where I would need to look to fix this issue.

backend/FwLite/LcmCrdt.Tests/Changes/JsonPatchChangeTests.cs

hahn-kev

this looks like a good start, this still some things to do. I've fixed a few things by changing the CreateSenseChange to only use the PartOfSpeech Id, and I've fixed some tests which were failing by always querying out the PartOfSpeech object in the relevant places.

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs

backend/FwLite/MiniLcm.Tests/BasicApiTestsBase.cs

backend/FwLite/MiniLcm/SyncHelpers/SenseSync.cs

backend/LfClassicData/LfClassicMiniLcmApi.cs

rmunn · 2025-01-08T20:20:44Z

@hahn-kev - Addressed your review comments so far. I still plan to make one more change (see #1350 (comment) for details) tomorrow to make GetPartOfSpeech make a single query instead of two, but I'm otherwise done with your suggested changes so far and ready to start working on frontend code.

github-actions · 2025-01-08T20:38:44Z

UI unit Tests

12 tests 12 ✅ 0s ⏱️
4 suites 0 💤
1 files 0 ❌

Results for commit 1e14f4c.

♻️ This comment has been updated with latest results.

rmunn · 2025-01-09T16:07:12Z

Rebased on top of current develop since some recent PRs were touching viewer-related code and I want to solve merge conflicts before they have a chance to happen.

rmunn · 2025-01-09T19:14:05Z

I now have the frontend working at first glance with the test Sena-3 project; screenshots in PR description above. I'm sure there are bugs that I'll reveal once I start trying to sync projects, but the Sena-3 test data has now been converted to use objects and all the parts of speech that Sena-3 references have been put into the data that entry-data.ts exports. Now I'll start actually using this with other test projects, to identify any further bugs. Then once I can't find any more obvious bugs, I'll mark the PR ready for review.

rmunn · 2025-01-09T22:18:28Z

@hahn-kev - How do I update DataModelSnapshotTests.VerifyDbModel.verified.txt so that the LcmCrdt.Tests.DataModelSnapshotTests.VerifyDbModel test will pass? My change to the type of Sense.PartOfSpeech is obviously what's causing this test failure here.

Besides that test, all backend tests passing except for one:

backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs(45): error TESTERROR:
FwLiteProjectSync.Tests.UpdateDiffTests.SenseDiffShouldUpdateAllFields (116ms): Error Message: Expected property before.PartOfSpeech to be MiniLcm.Models.PartOfSpeech
{
    DeletedAt = <2025-01-09 03:56:58.6540783 -5h>,
    Id = {0edb0965-c108-31ab-4804-e50a1a62acd6},
    Name = {[MiniLcm.Models.WritingSystemId
        {
            Code = "jbu"
        }] = "Music", [MiniLcm.Models.WritingSystemId
        {
            Code = "nxk"
        }] = "Unbranded", [MiniLcm.Models.WritingSystemId
        {
            Code = "oar"
        }] = "deposit"
    }
    ,
    Predefined = True
}, but found <null>.

I might need some help figuring that one out too; it's not clear to me why before.PartOfSpeech is ending up null here.

This is getting very close to being ready.

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs

backend/LfClassicData/LfClassicMiniLcmApi.cs

hahn-kev

Hum, I just realized an issue. Sense now has 2 properties which can specify the PartOfSpeech Id, either the PartOfSpeechId property or the PartOfSpeech object Id property. This means the API consumer could specify different Ids in each property, either when creating or updating the PoS. I believe our frontend only changes the PosId on the sense, not the object so when we send the Sense to be updated it also has this conflict. I think the most sensible thing would be to validate that the Ids match on both and if they don't then we throw a validation error. We could make an exception and say that if the PoS object is null and the PoSId on the sense is not then we use the value on the Sense, this would work best with our existing frontend as we would just need to set the object to null before we send it to the api. This won't represent a problem for FW Sync as both properties will always be the same.

hahn-kev · 2025-01-10T03:34:57Z

@hahn-kev - How do I update DataModelSnapshotTests.VerifyDbModel.verified.txt so that the LcmCrdt.Tests.DataModelSnapshotTests.VerifyDbModel test will pass? My change to the type of Sense.PartOfSpeech is obviously what's causing this test failure here.

Besides that test, all backend tests passing except for one:
backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs(45): error TESTERROR:
FwLiteProjectSync.Tests.UpdateDiffTests.SenseDiffShouldUpdateAllFields (116ms): Error Message: Expected property before.PartOfSpeech to be MiniLcm.Models.PartOfSpeech
{
    DeletedAt = <2025-01-09 03:56:58.6540783 -5h>,
    Id = {0edb0965-c108-31ab-4804-e50a1a62acd6},
    Name = {[MiniLcm.Models.WritingSystemId
        {
            Code = "jbu"
        }] = "Music", [MiniLcm.Models.WritingSystemId
        {
            Code = "nxk"
        }] = "Unbranded", [MiniLcm.Models.WritingSystemId
        {
            Code = "oar"
        }] = "deposit"
    }
    ,
    Predefined = True
}, but found <null>.
I might need some help figuring that one out too; it's not clear to me why before.PartOfSpeech is ending up null here.

This is getting very close to being ready.

For the verification test you copy/rename DataModelSnapshotTests.VerifyDbModel.received.txt to DataModelSnapshotTests.VerifyDbModel.verified.txt overwriting the existing one. You can read about verify here if you want.

As for the test, it's ending up null because we don't touch the PartOfSpeech property at all in the DiffUpdate. I wouldn't worry too much about it as that's a fair bit of code that needs to be cleaned up since we refactored our update code with the whole before after convention. You can just configure that test to ignore the PartOfSpeech property like similar to the other properties it's excluding.

backend/FwLite/MiniLcm.Tests/CreateEntryTestsBase.cs

rmunn · 2025-01-10T14:11:02Z

I think the most sensible thing would be to validate that the Ids match on both and if they don't then we throw a validation error.

Yep, now that #1344 is approved I'm planning to rebase this PR on top of the validation and then add some validation to make sure that PartOfSpeech.Id == PartOfSpeechId (and handle null / Guid.Empty sensibly). Glad to see we're on the same page as far as the design.

These tests run a Send/Receive as part of the test and are too slow to be considered unit tests.

…her PartOfSpeechId or PartOfSpeech are defined

…ting a sense

Also adjust test to expect an exception for missing parts of speech.

Auto-generated types for frontend have picked up the change of Sense.PartOfSpeech from a string to an object, so now it's time to change the frontend code accordingly.

More UI changes will ripple out from here

Having the complete list ensures that the Sena-3 test project's grammar picker, and the display of the parts of speech, will work.

Unit tests expects a specific exception type, so we must be consistent

The SenseDiffToUpdate method specifically excludes the PartOfSpeech object now, so the test shouldn't expect PartOfSpeech updated.

# Conflicts: # backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt

hahn-kev · 2025-01-14T05:51:57Z

@rmunn I've pushed a fix for the test that was failing (the sync tests operate on the same data, so one test failure can effect others). The test in question was using a part of speech before it was created which was causing the problem. I also fixed some issues in the CreateSenseChange as it would add references to Parts of speech and Semantic domains even if they were deleted, that's fixed now too.

rmunn · 2025-01-14T17:16:04Z

@hahn-kev - Thanks to your fixes, this seems to be working now: all tests are passing, both in GHA and locally on my machine, and I've addressed all the review comments you left earlier. Any other changes needed before this is ready to merge?

backend/LfClassicData/LfClassicMiniLcmApi.cs

hahn-kev

looks good, take a look at the one change I did to avoid a concurrency issue

rmunn mentioned this pull request Jan 7, 2025

Validate MiniLcm types #1344

Merged

hahn-kev reviewed Jan 8, 2025

View reviewed changes

backend/FwLite/LcmCrdt.Tests/Changes/JsonPatchChangeTests.cs Outdated Show resolved Hide resolved

hahn-kev requested changes Jan 8, 2025

View reviewed changes

rmunn force-pushed the feat/crdt-part-of-speech-object-in-senses branch from dd04723 to dc31a2f Compare January 9, 2025 16:06

hahn-kev reviewed Jan 10, 2025

View reviewed changes

backend/FwLite/LcmCrdt/Changes/SetPartOfSpeechChange.cs Show resolved Hide resolved

hahn-kev reviewed Jan 10, 2025

View reviewed changes

backend/LfClassicData/LfClassicMiniLcmApi.cs Show resolved Hide resolved

hahn-kev reviewed Jan 10, 2025

View reviewed changes

backend/FwLite/MiniLcm.Tests/CreateEntryTestsBase.cs Outdated Show resolved Hide resolved

rmunn and others added 11 commits January 10, 2025 10:08

Backend changes to make PartOfSpeech an object

d1e54aa

Mark FwLiteProjectSync tests as integration tests

e92c76c

These tests run a Send/Receive as part of the test and are too slow to be considered unit tests.

Fix a couple of tests broken by PoS type change

7385dfe

Better use of Task.WhenAll

1788d74

Fix failing integration tests

c5f358d

Fix up part of speech creation in AutoFaker

a62cb9a

don't store PartOfSpeech objects in CreateSenseChange

7cdd697

load part of speech when loading senses

329a9c5

ensure EntryReadyForCreation always creates a part of speech when eit…

9d60316

…her PartOfSpeechId or PartOfSpeech are defined

fix UpdateSensePartOfSpeech as now PartOfSpeech gets used when crea…

6fbbf4c

…ting a sense

When part of speech GUID not found, should throw

64029e3

Also adjust test to expect an exception for missing parts of speech.

rmunn added 12 commits January 10, 2025 10:08

Address various review comments

bb0eec4

Change to frontend generated type for PartOfSpeech

879f2d0

Auto-generated types for frontend have picked up the change of Sense.PartOfSpeech from a string to an object, so now it's time to change the frontend code accordingly.

Update entry-data.ts and utils.ts to use IPartOfSpeech

73a17b9

More UI changes will ripple out from here

More part of speech frontend changes

a36d2a2

Fix old typo in function

261d8cb

Update test sena-3 data with new partOfSpeech objects

24c1c61

Add other parts of speech found in Sena-3 test data

a6b8b02

Having the complete list ensures that the Sena-3 test project's grammar picker, and the display of the parts of speech, will work.

Get CanCreateEntry_AutoFaker test passing

f652112

Consistent exceptions between FwData and CRDT APIs

9b96ec5

Unit tests expects a specific exception type, so we must be consistent

VerifyDbModel test now expects PartOfSpeech object

b333bc1

Fix SenseDiffShouldUpdateAllFields test

e4521c5

The SenseDiffToUpdate method specifically excludes the PartOfSpeech object now, so the test shouldn't expect PartOfSpeech updated.

Can now validate PartOfSpeech GUIDs safely

93bc1b5

rmunn force-pushed the feat/crdt-part-of-speech-object-in-senses branch from dd0c5a9 to 93bc1b5 Compare January 10, 2025 16:13

Add validation check for PartOfSpeechId

4890857

rmunn force-pushed the feat/crdt-part-of-speech-object-in-senses branch from 00335e4 to 4890857 Compare January 10, 2025 18:23

rmunn self-assigned this Jan 10, 2025

hahn-kev added 3 commits January 14, 2025 11:27

fix test which was trying to use a PartOfSpeech before it was created

46135af

handle deleted semantic domains and part of speech in CreateSenseChange

e515f4a

Merge branch 'develop' into feat/crdt-part-of-speech-object-in-senses

f761055

# Conflicts: # backend/FwLite/LcmCrdt.Tests/DataModelSnapshotTests.VerifyDbModel.verified.txt

rmunn requested a review from hahn-kev January 14, 2025 17:16

rmunn marked this pull request as ready for review January 14, 2025 23:08

hahn-kev reviewed Jan 15, 2025

View reviewed changes

backend/LfClassicData/LfClassicMiniLcmApi.cs Outdated Show resolved Hide resolved

changed code converting senses to avoid a cache stampede

e65013c

hahn-kev approved these changes Jan 15, 2025

View reviewed changes

rmunn added 2 commits January 15, 2025 09:14

Merge branch 'develop' into feat/crdt-part-of-speech-object-in-senses

45e7453

Add FwLite DB migration for part of speech object

1e14f4c

rmunn merged commit bb51178 into develop Jan 15, 2025
24 checks passed

rmunn deleted the feat/crdt-part-of-speech-object-in-senses branch January 15, 2025 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Sense.PartOfSpeech an object instead of a string #1350

Make Sense.PartOfSpeech an object instead of a string #1350

rmunn commented Jan 7, 2025 •

edited

Loading

rmunn commented Jan 7, 2025

github-actions bot commented Jan 7, 2025 •

edited

Loading

rmunn commented Jan 7, 2025

rmunn commented Jan 8, 2025

hahn-kev left a comment

rmunn commented Jan 8, 2025

github-actions bot commented Jan 8, 2025 •

edited

Loading

rmunn commented Jan 9, 2025

rmunn commented Jan 9, 2025

rmunn commented Jan 9, 2025

hahn-kev left a comment

hahn-kev commented Jan 10, 2025

rmunn commented Jan 10, 2025

hahn-kev commented Jan 14, 2025

rmunn commented Jan 14, 2025

hahn-kev left a comment

Make Sense.PartOfSpeech an object instead of a string #1350

Make Sense.PartOfSpeech an object instead of a string #1350

Conversation

rmunn commented Jan 7, 2025 • edited Loading

rmunn commented Jan 7, 2025

github-actions bot commented Jan 7, 2025 • edited Loading

C# Unit Tests

rmunn commented Jan 7, 2025

rmunn commented Jan 8, 2025

hahn-kev left a comment

Choose a reason for hiding this comment

rmunn commented Jan 8, 2025

github-actions bot commented Jan 8, 2025 • edited Loading

UI unit Tests

rmunn commented Jan 9, 2025

rmunn commented Jan 9, 2025

rmunn commented Jan 9, 2025

hahn-kev left a comment

Choose a reason for hiding this comment

hahn-kev commented Jan 10, 2025

rmunn commented Jan 10, 2025

hahn-kev commented Jan 14, 2025

rmunn commented Jan 14, 2025

hahn-kev left a comment

Choose a reason for hiding this comment

rmunn commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 7, 2025 •

edited

Loading

github-actions bot commented Jan 8, 2025 •

edited

Loading