Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minilcm diff API #1181

Open
wants to merge 27 commits into
base: develop
Choose a base branch
from
Open

Minilcm diff API #1181

wants to merge 27 commits into from

Conversation

hahn-kev
Copy link
Collaborator

closes #1127

  • uses the new harmony version to put a commit id on each object
  • creates a new UpdateEntry(Entry entry) api for updating using the version to ensure we don't stomp on changes
  • write some fuzzing tests using randomly generated entries for create and update tests

hahn-kev and others added 25 commits October 17, 2024 14:54
was current/previous now before after
# Conflicts:
#	backend/FwLite/FwDataMiniLcmBridge/Api/UpdateProxy/UpdateSenseProxy.cs
#	backend/FwLite/FwLiteProjectSync.Tests/UpdateDiffTests.cs
#	backend/FwLite/LcmCrdt/CrdtMiniLcmApi.cs
#	backend/FwLite/MiniLcm/Models/Entry.cs
# Conflicts:
#	.idea/.idea.LexBox/.idea/dataSources.xml
#	backend/FwLite/FwLiteProjectSync.Tests/Fixtures/SyncFixture.cs

update2.LexemeForm["es"] = "updated again";
Func<Task> action = async () => await Api.UpdateEntry(update2);
await action.Should().ThrowAsync<VersionInvalidException>();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is different from the CRDT version, is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what CRDT's are all about right?! 🙂 👍

Copy link

github-actions bot commented Oct 30, 2024

C# Unit Tests

75 tests  ±0   75 ✅ ±0   5s ⏱️ ±0s
13 suites ±0    0 💤 ±0 
 1 files   ±0    0 ❌ ±0 

Results for commit e402294. ± Comparison against base commit a84c509.

♻️ This comment has been updated with latest results.

}

[Fact]
public async Task UpdateEntry_CanUseSameVersionMultipleTimes()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as with a similar version in FW, this is different behavior from the CRDT Version

.SingleAsyncEF();
//todo this will not work for properties not in the snapshot, but we don't have a way to get the full snapshot yet
//todo also, add api for getting an entity at a specific commit
var snapshot = await dataModel.GetEntitySnapshotAtTime(commitHybridDate.DateTime.AddSeconds(1), entry.Id);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I plan to fix this up so we can pass in the commit Id and get back the entry

var snapshot = await dataModel.GetEntitySnapshotAtTime(commitHybridDate.DateTime.AddSeconds(1), entry.Id);
var before = snapshot?.Entity.DbObject as Entry;
ArgumentNullException.ThrowIfNull(before);
//workaround to avoid syncing senses, which are not in the snapshot
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to resolve this issue

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we basically have to replicate this?
image

if (createComplexFormTypes) await CreateComplexFormTypes(entry.ComplexFormTypes, api);
foreach (var sense in entry.Senses)
{
sense.EntryId = entry.Id;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to have to think about this when using our faker, but we should write a test and decide what we want to happen if the EntryId does not match on the sense, maybe if it's empty then we set it, but if it's not the same should we throw?

//generated entries won't have the expected ids, so fix them up here
if (isComponent)
{
complexFormComponent.ComplexFormEntryId = entryId;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same for complex form stuff, if the id doesn't match what it should be do throw?

{
if (context.Instance is IObjectWithId obj)
{
obj.DeletedAt = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone calls update with a deleted object what should we do? throw? delete it?

//todo limitation of fwdata prevents us from specifying the complex form type ahead of time
foreach (var entryComplexFormType in entry.ComplexFormTypes)
{
entryComplexFormType.Id = Guid.Empty;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@myieye myieye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made a bit of progress

var lexEntry = EntriesRepository.GetObject(complexFormComponent.ComplexFormEntryId);
AddComplexFormComponent(lexEntry, complexFormComponent);
});
return Task.FromResult(ToComplexFormComponents(EntriesRepository.GetObject(complexFormComponent.ComplexFormEntryId)).Single(c => c.ComponentEntryId == complexFormComponent.ComponentEntryId));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A complex form can reference both an entry itself as well as any number of senses of that entry. That's how the FW UI works at least, so the FWL UI does the same.

Suggested change
return Task.FromResult(ToComplexFormComponents(EntriesRepository.GetObject(complexFormComponent.ComplexFormEntryId)).Single(c => c.ComponentEntryId == complexFormComponent.ComponentEntryId));
return Task.FromResult(ToComplexFormComponents(EntriesRepository.GetObject(complexFormComponent.ComplexFormEntryId)).Single(c => c.ComponentEntryId == complexFormComponent.ComponentEntryId && c.ComponentSenseId == complexFormComponent.ComponentSenseId));

@@ -64,14 +64,14 @@ public override IList<SemanticDomain> SemanticDomains
new UpdateListProxy<SemanticDomain>(
semanticDomain =>
{
#pragma warning disable VSTHRD002
if (semanticDomain.Id != default && lexboxLcmApi.GetLcmSemanticDomain(semanticDomain.Id) is { } lcmSemanticDomain)
sense.SemanticDomainsRC.Add(lcmSemanticDomain);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does Remove use the lexboxLcmApi, but Add doesn't?

}
]
});
Entry after = (Entry) component.Copy();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting a lot of mileage out of .Copy() 😀

@@ -49,16 +49,15 @@ public async Task InitializeAsync()
.ProjectsFolder;
if (Path.Exists(projectsFolder)) Directory.Delete(projectsFolder, true);
Directory.CreateDirectory(projectsFolder);
_services.ServiceProvider.GetRequiredService<IProjectLoader>()
var lcmCache = _services.ServiceProvider.GetRequiredService<IProjectLoader>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks unintentional as I don't think it's being used.

beforeComponents,
afterComponents,
//we can't use the ID as there's none defined by Fw so it won't work as a sync key
component => (component.ComplexFormEntryId, component.ComponentEntryId, component.ComponentSenseId),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this will never result in a replace operation, because the identity is equivalent to the value, so the collection differ will call replace for all unchanged components, bceause the id is the same and then the replace logic starting on L83 will do nothing, because the value is the same.

You probably know that and it's the simplest way to keep everything happy for now.

This is where a more sophisticated list differ would be nice in the future. E.g. if the before and after lists are the same length and only the nth object changed, then a replace probably makes sense even if the IDs are different. But then we'd be disregarding Guids that the client generated, so that might be bad.

public static UpdateObjectInput<Entry>? EntryDiffToUpdate(Entry beforeEntry, Entry afterEntry)
{
JsonPatchDocument<Entry> patchDocument = new();
patchDocument.Operations.AddRange(MultiStringDiff.GetMultiStringDiff<Entry>(nameof(Entry.LexemeForm), beforeEntry.LexemeForm, afterEntry.LexemeForm));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts on using reflection for this?

patchDocument.Operations.AddRange(MultiStringDiff.GetMultiStringDiff<Sense>(nameof(Sense.Definition),
beforeSense.Definition,
afterSense.Definition));
if (beforeSense.PartOfSpeech != afterSense.PartOfSpeech)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having both PartofSpeech and PartOfSpeechId is code debt, right?
I feel like it was a quick hack somewhere along the line. Should there be an issue to clean that up?

if (complexFormComponent.ComplexFormEntryId == default) complexFormComponent.ComplexFormEntryId = entry.Id;
if (complexFormComponent.ComponentEntryId == complexFormComponent.ComplexFormEntryId)
{
throw new InvalidOperationException($"Complex form component {complexFormComponent} has the same component id as its complex form");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these exceptions are being thrown in GetEntry and are testing for things we don't expect to be possible right?
They're also slow, because they're behind db queries.

Shouldn't this validation essentially be dev-mode asserts?

var snapshot = await dataModel.GetEntitySnapshotAtTime(commitHybridDate.DateTime.AddSeconds(1), entry.Id);
var before = snapshot?.Entity.DbObject as Entry;
ArgumentNullException.ThrowIfNull(before);
//workaround to avoid syncing senses, which are not in the snapshot
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we basically have to replicate this?
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change MiniLcm update apis
3 participants