You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Null notes/chords in a Chord or Phrase are accepted, then crash later with NullReferenceException in ToString() and SaveAsync #187
Chord and Phrase check their collections for null but never check the elements inside them:
Chord(IEnumerable<Note>) is in Keybinding/Models/MusicalTypes.cs around lines 150-159.
Phrase(IEnumerable<Chord>) is around lines 359-368.
Phrase(Chord) (around line 374) calls this([chord]) with no Ensure.NotNull, unlike Chord(Note).
The [JsonConstructor] overloads of Chord, Phrase and Profile (Models/Profile.cs:41-50) all go through these constructors.
A bad value is therefore accepted when it is built and only fails later, somewhere unrelated.
Reproduction
Each case below was run against the current main:
JsonSerializer.Deserialize<Chord>("{\"Notes\":[null]}") succeeds, giving Notes.Count == 1 and Notes[0] == null. Calling ToString() on the result then throws NullReferenceException in IsModifierNote.
{"Notes":[null,{"Key":"A"}]} throws NullReferenceException from the OrderBy key selector instead of a JsonException.
{"Notes":null} throws ArgumentNullException and {"Notes":[]} throws ArgumentException. Callers that catch JsonException miss both.
new Phrase((Chord)null!) and Deserialize<Phrase>("{\"Sequence\":[null]}") both succeed, and ToString() then throws NullReferenceException.
A profile loaded from {"Id":"a","Name":"A","Chords":{"x.y":{"Notes":[null]}}} reports ChordCount == 1. Once it is registered, KeybindingManager.SaveAsync → JsonKeybindingRepository.SaveProfileAsync throws NullReferenceException at n.ToString() (Services/JsonKeybindingRepository.cs:66), and the save fails part-way through.
NoteJsonConverter already wraps invalid keys in JsonException (Models/NoteJsonConverter.cs:318-328). That is the contract the other types should follow.
Suggested fix
In the Chord and Phrase constructors, reject null elements with ArgumentException before Distinct/OrderBy.
Add Ensure.NotNull(chord) to Phrase(Chord).
In the private [JsonConstructor] overloads, catch ArgumentException and rethrow it as JsonException (inner exception kept), the same way NoteJsonConverter does.
Acceptance criteria
Deserializing Chord or Phrase JSON with null, missing or empty Notes/Sequence throws JsonException.
Constructing either type with a null element throws ArgumentException.
Priority: Medium. A corrupt or hand-edited profile containing a null note loads without complaint and later makes SaveAsync throw part-way through, which can leave stored profiles partly written. Callers that catch JsonException also miss the ArgumentException and NullReferenceException thrown on bad input.
Area / suggested assignee: the Chord and Phrase constructors and their [JsonConstructor] overloads (Models/MusicalTypes.cs, Models/Profile.cs). Owner: @matt-edmondson
Next step: reject null elements with ArgumentException, add Ensure.NotNull to Phrase(Chord), and wrap the JSON constructors' failures in JsonException the way NoteJsonConverter does. Add regression tests for each case listed.
Activity
matt-edmondson commentedon Oct 18, 2026
Triage
SaveAsyncthrow part-way through, which can leave stored profiles partly written. Callers that catchJsonExceptionalso miss theArgumentExceptionandNullReferenceExceptionthrown on bad input.ChordandPhraseconstructors and their[JsonConstructor]overloads (Models/MusicalTypes.cs,Models/Profile.cs). Owner: @matt-edmondsonCommand's constructor only). Related to One wrong-typed field in a single profiles.json/commands.json entry discards every stored entry, and the next save erases them #168 (one bad entry discards every stored entry) and A torn or corrupt profiles.json loads as zero profiles and the next SaveAsync overwrites it for good (non-atomic saves, corrupt file not kept) #157 (non-atomic saves). Together they make the profile load/save path fragile.ArgumentException, addEnsure.NotNulltoPhrase(Chord), and wrap the JSON constructors' failures inJsonExceptionthe wayNoteJsonConverterdoes. Add regression tests for each case listed.Generated by Claude Code