Skip to content

Commit 6bd027b

Browse files
myieyehahn-kev
myieye
and
hahn-kev
authored
Fix NPEs when using gridify in fw-data projects (#1884)
* Map nulls to empty string and normalize filtering for null to null or empty * write an explicit test that LcmHelpers.PickText does not return null * use string.Empty instead of "", change PickText return type to not be null * Gridify: turn off null handling and patch it for collections * Test gridify null and empty string handling for multi-strings * Fix and test empty string round tripping * Tidy up strings --------- Co-authored-by: Kevin Hahn <[email protected]>
1 parent 7882f12 commit 6bd027b

File tree

10 files changed

+165
-22
lines changed

10 files changed

+165
-22
lines changed

‎backend/FwLite/FwDataMiniLcmBridge.Tests/LcmHelpersTests.cs‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
using FwDataMiniLcmBridge.Api;
2+
using FwDataMiniLcmBridge.Tests.Fixtures;
3+
using MiniLcm.Models;
4+
using SIL.LCModel;
5+
6+
namespace FwDataMiniLcmBridge.Tests;
7+
8+
[Collection(ProjectLoaderFixture.Name)]
9+
public class LcmHelpersTests
10+
{
11+
private FwDataMiniLcmApi _fwDataMiniLcmApi;
12+
13+
public LcmHelpersTests(ProjectLoaderFixture fixture)
14+
{
15+
_fwDataMiniLcmApi = fixture.NewProjectApi("lcm-helpers-test", "en", "en");
16+
}
17+
18+
[Fact]
19+
public async Task PickTextMustNotReturnNull()
20+
{
21+
var id = Guid.NewGuid();
22+
await _fwDataMiniLcmApi.CreateEntry(new Entry(){Id = id});
23+
var entry = (ILexEntry) _fwDataMiniLcmApi.Cache.ServiceLocator.ObjectRepository.GetObject(id);
24+
//must not return null, because of https://github.com/sillsdev/languageforge-lexbox/issues/1874
25+
entry.PickText(entry.LexemeFormOA.Form, "en").Should().NotBeNull().And.BeEmpty();
26+
}
27+
}

‎backend/FwLite/FwDataMiniLcmBridge.Tests/MiniLcmTests/QueryEntryTests.cs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,3 +10,12 @@ protected override Task<IMiniLcmApi> NewApi()
1010
return Task.FromResult<IMiniLcmApi>(fixture.NewProjectApi("query-entry-test", "en", "en"));
1111
}
1212
}
13+
14+
[Collection(ProjectLoaderFixture.Name)]
15+
public class NullAndEmptyQueryEntryTests(ProjectLoaderFixture fixture) : NullAndEmptyQueryEntryTestsBase
16+
{
17+
protected override Task<IMiniLcmApi> NewApi()
18+
{
19+
return Task.FromResult<IMiniLcmApi>(fixture.NewProjectApi("null-and-empty-query-entry-test", "en", "en"));
20+
}
21+
}

‎backend/FwLite/FwDataMiniLcmBridge/Api/FwDataMiniLcmApi.cs‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -682,7 +682,10 @@ private MultiString FromLcmMultiString(ITsMultiString? multiString)
682682
var wsId = GetWritingSystemId(ws);
683683
if (!wsId.IsAudio)
684684
{
685-
result.Values.Add(wsId, tsString.Text);
685+
// Text is null if TsStringUtils.MakeString was called with an empty string.
686+
// So, we map it back for consistent round-tripping and
687+
// so we can continue to assume that MultiStrings never have null values.
688+
result.Values.Add(wsId, tsString.Text ?? string.Empty);
686689
}
687690
else
688691
{
@@ -1137,7 +1140,7 @@ private void UpdateLcmMultiString(ITsMultiString multiString, RichMultiString ne
11371140
{
11381141
foreach (var (ws, value) in newMultiString)
11391142
{
1140-
multiString.SetString(this, ws, value);;
1143+
multiString.SetString(this, ws, value);
11411144
}
11421145
}
11431146

‎backend/FwLite/FwDataMiniLcmBridge/Api/LcmHelpers.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -142,10 +142,10 @@ internal static int GetWritingSystemHandle(this LcmCache cache, WritingSystemId
142142
return lcmWs.Handle;
143143
}
144144

145-
internal static string? PickText(this ICmObject obj, ITsMultiString multiString, string ws)
145+
internal static string PickText(this ICmObject obj, ITsMultiString multiString, string ws)
146146
{
147147
var wsHandle = obj.Cache.GetWritingSystemHandle(ws);
148-
return multiString.get_String(wsHandle)?.Text ?? null;
148+
return multiString.get_String(wsHandle)?.Text ?? string.Empty;
149149
}
150150

151151
internal static IMoStemAllomorph CreateLexemeForm(this LcmCache cache)

‎backend/FwLite/FwDataMiniLcmBridge/LexEntryFilterMapProvider.cs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ public class LexEntryFilterMapProvider : EntryFilterMapProvider<ILexEntry>
1616
//don't convert this to a method group, aka keep it as a lambda as that's what Gridify expects
1717
// ReSharper disable once ConvertClosureToMethodGroup
1818
.Select(domain => LcmHelpers.GetSemanticDomainCode(domain));
19-
public override Func<string, object>? EntrySensesSemanticDomainsConverter => EntryFilter.NormalizeEmptyToNullString<ICmSemanticDomain>;
19+
public override Func<string, object>? EntrySensesSemanticDomainsConverter => EntryFilter.NormalizeEmptyToNull<ICmSemanticDomain>;
2020
public override Expression<Func<ILexEntry, object?>> EntrySensesExampleSentences => e => e.AllSenses.Select(s => EmptyToNull(s.ExamplesOS));
2121
public override Expression<Func<ILexEntry, string, object?>> EntrySensesExampleSentencesSentence => (entry, ws) =>
2222
entry.AllSenses.SelectMany(s => s.ExamplesOS).Select(example => example.PickText(example.Example, ws));
@@ -33,5 +33,5 @@ public class LexEntryFilterMapProvider : EntryFilterMapProvider<ILexEntry>
3333
public override Expression<Func<ILexEntry, string, object?>> EntryCitationForm => (entry, ws) => entry.PickText(entry.CitationForm, ws);
3434
public override Expression<Func<ILexEntry, string, object?>> EntryLiteralMeaning => (entry, ws) => entry.PickText(entry.LiteralMeaning, ws);
3535
public override Expression<Func<ILexEntry, object?>> EntryComplexFormTypes => e => EmptyToNull(e.ComplexFormEntryRefs.SelectMany(r => r.ComplexEntryTypesRS));
36-
public override Func<string, object>? EntryComplexFormTypesConverter => EntryFilter.NormalizeEmptyToNullString<ILexEntryType>;
36+
public override Func<string, object>? EntryComplexFormTypesConverter => EntryFilter.NormalizeEmptyToNull<ILexEntryType>;
3737
}

‎backend/FwLite/LcmCrdt.Tests/MiniLcmTests/QueryEntryTests.cs‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
using Bogus;
33
using LcmCrdt.FullTextSearch;
44
using LinqToDB;
5-
using LinqToDB.EntityFrameworkCore;
65
using Microsoft.EntityFrameworkCore;
76
using MiniLcm.Culture;
87
using Xunit.Abstractions;
@@ -67,3 +66,21 @@ public override async Task DisposeAsync()
6766
await _fixture.DisposeAsync();
6867
}
6968
}
69+
70+
public class NullAndEmptyQueryEntryTests : NullAndEmptyQueryEntryTestsBase
71+
{
72+
private readonly MiniLcmApiFixture _fixture = new();
73+
74+
protected override async Task<IMiniLcmApi> NewApi()
75+
{
76+
await _fixture.InitializeAsync();
77+
var api = _fixture.Api;
78+
return api;
79+
}
80+
81+
public override async Task DisposeAsync()
82+
{
83+
await base.DisposeAsync();
84+
await _fixture.DisposeAsync();
85+
}
86+
}

‎backend/FwLite/LcmCrdt/EntryFilterMapProvider.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ public class EntryFilterMapProvider : EntryFilterMapProvider<Entry>
1212
e => e.Senses.SelectMany(s => s.SemanticDomains).Select(sd => Json.Value(sd, sd => sd.Code));
1313
public override Func<string, object>? EntrySensesSemanticDomainsConverter =>
1414
//linq2db treats Sense.SemanticDomains as a table, if we use "null" then it'll write the query we want
15-
EntryFilter.NormalizeEmptyToNullString<SemanticDomain>;
15+
EntryFilter.NormalizeEmptyToNull<SemanticDomain>;
1616
public override Expression<Func<Entry, object?>> EntrySensesExampleSentences => e => e.Senses.Select(s => s.ExampleSentences);
1717
public override Expression<Func<Entry, string, object?>> EntrySensesExampleSentencesSentence =>
1818
(e, ws) => e.Senses.SelectMany(s => s.ExampleSentences).Select(example => Json.Value(example.Sentence, ms => ms[ws]));

‎backend/FwLite/MiniLcm.Tests/QueryEntryTestsBase.cs‎

Lines changed: 72 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ public abstract class QueryEntryTestsBase : MiniLcmTestBase
1010
private readonly string Peach = "Peach";
1111
private readonly string Banana = "Banana";
1212
private readonly string Kiwi = "Kiwi";
13+
private readonly string Null_LexemeForm = string.Empty; // nulls get normalized to empty strings
1314

1415
private static readonly AutoFaker Faker = new(AutoFakerDefault.Config);
1516

@@ -87,13 +88,15 @@ await Api.CreateEntry(new Entry()
8788
}
8889
]
8990
});
91+
// null / missing key - exposes potential NPEs
92+
await Api.CreateEntry(new Entry());
9093
}
9194

9295
[Fact]
9396
public async Task CanFilterToMissingSenses()
9497
{
9598
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "Senses=null" })).ToArrayAsync();
96-
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple);
99+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, Null_LexemeForm);
97100
}
98101

99102
[Fact]
@@ -106,7 +109,7 @@ public async Task CanFilterToNotMissingSenses()
106109
[Fact]
107110
public async Task CanFilterToMissingPartOfSpeech()
108111
{
109-
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "Senses.PartOfSpeechId=null" })).ToArrayAsync();
112+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "Senses.PartOfSpeechId=" })).ToArrayAsync();
110113
//does not include entries with no senses
111114
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Peach);
112115
}
@@ -145,14 +148,14 @@ public async Task CanFilterSemanticDomainCodeContains()
145148
public async Task CanFilterToMissingComplexFormTypes()
146149
{
147150
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "ComplexFormTypes=null" })).ToArrayAsync();
148-
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, Banana, Kiwi);
151+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, Banana, Kiwi, Null_LexemeForm);
149152
}
150153

151154
[Fact]
152155
public async Task CanFilterToMissingComplexFormTypesWithEmptyArray()
153156
{
154157
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "ComplexFormTypes=[]" })).ToArrayAsync();
155-
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, Banana, Kiwi);
158+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, Banana, Kiwi, Null_LexemeForm);
156159
}
157160

158161
[Fact]
@@ -187,11 +190,13 @@ public async Task CanFilterLexemeFormContains()
187190
public async Task CanFilterGlossNull()
188191
{
189192
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "Senses.Gloss[en]=null" })).ToArrayAsync();
190-
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Peach);
193+
/// No entries have a gloss of "null"
194+
/// <see cref="Filtering.EntryFilter.NewMapper"/> and <see cref="NullAndEmptyQueryEntryTestsBase"/>
195+
results.Select(e => e.LexemeForm["en"]).Should().BeEmpty();
191196
}
192197

193198
[Fact]
194-
public async Task CanFilterGlossEmpty()
199+
public async Task CanFilterGlossEmptyOrNull()
195200
{
196201
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "Senses.Gloss[en]=" })).ToArrayAsync();
197202
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Peach);
@@ -317,3 +322,64 @@ public async Task HeadwordOrder(string searchTerm, string wordsAndGlosses, strin
317322
string.Join(",", result).Should().Be(expectedOrder);
318323
}
319324
}
325+
326+
// A seperate class to preserve the readability of the results in the main test class
327+
public abstract class NullAndEmptyQueryEntryTestsBase : MiniLcmTestBase
328+
{
329+
private readonly string Apple = "Apple";
330+
private readonly string Null = string.Empty; // nulls get normalized to empty strings
331+
private readonly string EmptyString = string.Empty;
332+
private readonly string NullString = "null";
333+
334+
public override async Task InitializeAsync()
335+
{
336+
await base.InitializeAsync();
337+
await Api.CreateEntry(new Entry() { LexemeForm = { { "en", Apple } } });
338+
// null / missing key
339+
await Api.CreateEntry(new Entry());
340+
// blank
341+
await Api.CreateEntry(new Entry() { LexemeForm = { ["en"] = EmptyString } });
342+
// null string
343+
await Api.CreateEntry(new Entry() { LexemeForm = { ["en"] = NullString } });
344+
}
345+
346+
[Fact]
347+
public async Task CanFilterIsNullOrEmpty()
348+
{
349+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "LexemeForm[en]=" })).ToArrayAsync();
350+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Null, EmptyString);
351+
}
352+
353+
[Fact]
354+
public async Task CanFilterIsNotNullOrEmpty()
355+
{
356+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "LexemeForm[en]!=" })).ToArrayAsync();
357+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(Apple, NullString);
358+
}
359+
360+
[Fact]
361+
public async Task CanFilterEqualsNullString()
362+
{
363+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "LexemeForm[en]=null" })).ToArrayAsync();
364+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(NullString);
365+
}
366+
367+
[Fact]
368+
public async Task CanFilterNotEqualsNullString()
369+
{
370+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "LexemeForm[en]!=null" })).ToArrayAsync();
371+
// Sadly the != operator isn't consistent, but it's an edge case that probably isn't crucial:
372+
// crdt logic: key exists (and/or value is not null, I'm not sure exactly) && LexemeForm[en] != "null"
373+
// fwdata logic: LexemeForm[en] != "null"
374+
// i.e. the entry that doesn't have LexemeForm[en] at all, is only included in the fwdata results
375+
results.Select(e => e.LexemeForm["en"]).Should().BeSubsetOf([Apple, Null, EmptyString]);
376+
results.Count().Should().BeInRange(2, 3);
377+
}
378+
379+
[Fact]
380+
public async Task CanFilterContainsNullString()
381+
{
382+
var results = await Api.GetEntries(new(Filter: new() { GridifyFilter = "LexemeForm[en]=*null" })).ToArrayAsync();
383+
results.Select(e => e.LexemeForm["en"]).Should().BeEquivalentTo(NullString);
384+
}
385+
}

‎backend/FwLite/MiniLcm.Tests/UpdateEntryTestsBase.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,17 @@ public async Task UpdateEntry_SupportsRemovingARichMultiStringWs()
8181
updatedEntry.Note.Should().NotContainKey("en");
8282
}
8383

84+
[Fact]
85+
public async Task UpdateEntry_RoundTripsEmptyStrings()
86+
{
87+
var entry = await Api.GetEntry(Entry1Id);
88+
ArgumentNullException.ThrowIfNull(entry);
89+
var before = entry.Copy();
90+
entry.CitationForm["en"] = string.Empty;
91+
var updatedEntry = await Api.UpdateEntry(before, entry);
92+
updatedEntry.CitationForm["en"].Should().Be(string.Empty);
93+
}
94+
8495
[Fact]
8596
public async Task UpdateEntry_SupportsSenseChanges()
8697
{

‎backend/FwLite/MiniLcm/Filtering/EntryFilter.cs‎

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,4 @@
1-
using System.Linq.Expressions;
2-
using Gridify;
3-
using Gridify.Syntax;
1+
using Gridify;
42
using MiniLcm.Models;
53

64
namespace MiniLcm.Filtering;
@@ -11,13 +9,14 @@ public static GridifyMapper<T> NewMapper<T>(EntryFilterMapProvider<T> provider)
119
{
1210
var mapper = new GridifyMapper<T>(false);
1311
mapper.Configuration.DisableCollectionNullChecks = true;
14-
mapper.AddMap(nameof(Entry.Senses), provider.EntrySenses);
12+
mapper.Configuration.AllowNullSearch = false; // We want LexemeForm=null to interpret null as a string rather than a null value, because it's a legitimate word.
13+
mapper.AddMap(nameof(Entry.Senses), provider.EntrySenses, NormalizeEmptyToNull<Sense>);
1514
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.SemanticDomains)}", provider.EntrySensesSemanticDomains, provider.EntrySensesSemanticDomainsConverter);
1615
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.SemanticDomains)}.{nameof(SemanticDomain.Code)}", provider.EntrySensesSemanticDomainsCode);
1716
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.PartOfSpeechId)}", provider.EntrySensesPartOfSpeechId);
1817
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.Gloss)}", provider.EntrySensesGloss);
1918
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.Definition)}", provider.EntrySensesDefinition);
20-
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.ExampleSentences)}", provider.EntrySensesExampleSentences);
19+
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.ExampleSentences)}", provider.EntrySensesExampleSentences, NormalizeEmptyToNull<ExampleSentence>);
2120
mapper.AddMap($"{nameof(Entry.Senses)}.{nameof(Sense.ExampleSentences)}.{nameof(ExampleSentence.Sentence)}", provider.EntrySensesExampleSentencesSentence);
2221

2322
mapper.AddMap(nameof(Entry.Note), provider.EntryNote);
@@ -31,13 +30,24 @@ public static GridifyMapper<T> NewMapper<T>(EntryFilterMapProvider<T> provider)
3130
//used by the database for json columns which are lists, we want to treat null as an empty list
3231
public static object NormalizeEmptyToEmptyList<T>(string value)
3332
{
34-
if (value is "null" or "" or "[]") return new List<T>();
33+
if (value is "null" or "[]") return new List<T>();
34+
// Note: we can't normalize from the empty string, because gridify has special IsNullOrDefault handling for that case
35+
// i.e. it always ignores the returned value and we can't do anything about that.
36+
// Throw because IsNullOrDefault won't work as expected
37+
if (value is "") throw new Exception("To filter for empty collections use [] or null.");
3538
throw new Exception($"Invalid value {value} for {typeof(T).Name}");
3639
}
3740

38-
public static object NormalizeEmptyToNullString<T>(string value)
41+
//used by the database for non-json collections
42+
public static object NormalizeEmptyToNull<T>(string value)
3943
{
40-
if (value is "null" or "" or "[]") return "null";
44+
// This essentially mimics AllowNullSearch = true, but lets us apply it to specific fields (i.e. only collections)
45+
if (value is "null" or "[]") return null!;
46+
// Note: we can't normalize from the empty string, because gridify has special IsNullOrDefault handling for that case
47+
// i.e. it always ignores the returned value and we can't do anything about that.
48+
// In this case it would actually work, because null is what the expression needs,
49+
// but throwing makes it consistent with json collections where we can't support ""/IsNullOrDefault.
50+
if (value is "") throw new Exception("To filter for empty collections use [] or null.");
4151
throw new Exception($"Invalid value {value} for {typeof(T).Name}");
4252
}
4353

0 commit comments

Comments
 (0)