Fix NullReferenceException in FindIndexOfNextParaMark#124
Open
aslakhellesoy wants to merge 2 commits intoJSv4:mainfrom
Open
Fix NullReferenceException in FindIndexOfNextParaMark#124aslakhellesoy wants to merge 2 commits intoJSv4:mainfrom
aslakhellesoy wants to merge 2 commits intoJSv4:mainfrom
Conversation
FindIndexOfNextParaMark assumed all elements in the comparison unit array were ComparisonUnitWord, but documents with bookmarkStart/ bookmarkEnd as direct children of w:body produce other ComparisonUnit types. The `as ComparisonUnitWord` cast returned null, and the subsequent .DescendantContentAtoms() call threw NullReferenceException. Added null guards for both the `as` cast (skip non-word units) and the LastOrDefault() result.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in WmlComparer.FindIndexOfNextParaMark when comparing documents that can produce non-ComparisonUnitWord entries in the comparison unit arrays (notably with body-level bookmarks), and adds a regression test + changelog entry.
Changes:
- Added guards in
FindIndexOfNextParaMarkto avoidNullReferenceExceptionwhen encountering non-word comparison units and whenLastOrDefault()returnsnull. - Added
WmlComparerBodyLevelBookmarkTeststo reproduce and prevent the crash. - Documented the fix in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Docxodus/WmlComparer.cs | Adds null/type guards in FindIndexOfNextParaMark to prevent a crash during LCS processing. |
| Docxodus.Tests/WmlComparerBodyLevelBookmarkTests.cs | Regression test ensuring comparisons don’t throw NullReferenceException for body-level bookmarks scenario. |
| CHANGELOG.md | Notes the fix in the “Fixed” section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Handle any ComparisonUnit with Contents (including ComparisonUnitGroup) in FindIndexOfNextParaMark, not just ComparisonUnitWord, since groups can also end with a w:pPr atom - Fix CHANGELOG wording to match the actual implementation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hello, and thanks for this great library!
I found a bug where an NRE is thrown when there are certain bookmarks. This PR has a new test that reproduces the bug, and a change to fix it. All other tests are passing.
Summary
NullReferenceExceptioninWmlComparer.FindIndexOfNextParaMarkwhen comparing documents where one hasbookmarkStart/bookmarkEndas direct children ofw:body(siblings ofw:prather than children)ComparisonUnitWord, but body-level bookmarks produce otherComparisonUnittypes. Theas ComparisonUnitWordcast returnednull, and.DescendantContentAtoms()threwascast (skip non-word units) and theLastOrDefault()resultTest plan
WmlComparerBodyLevelBookmarkTestswith test documents that reproduce the crashNullReferenceExceptionatFindIndexOfNextParaMark