Root element for generated parts should be nullable by tomjebo · Pull Request #1922 · dotnet/Open-XML-SDK

tomjebo · 2025-04-25T00:51:26Z

Changes root elements for generated parts to be nullable.
Fixes #959 and #995

github-actions · 2025-04-25T01:07:00Z

Test Results

56 files ± 0 56 suites ±0 1h 1m 53s ⏱️ + 4m 40s
2 050 tests ± 0 2 047 ✅ ± 0 3 💤 ±0 0 ❌ ±0
32 354 runs +362 32 318 ✅ +362 36 💤 ±0 0 ❌ ±0

Results for commit 4cd86d7. ± Comparison against base commit b127a66.

♻️ This comment has been updated with latest results.

Copilot

Pull Request Overview

This pull request makes root elements for generated parts nullable and updates related tests and generator code accordingly. Key changes include:

Updating test files to use null‐forgiving operators where necessary.
Adding explicit null checks in sample utility and presentation helper methods.
Adjusting generator code with nullable annotations and a new [DisallowNull] attribute.

Reviewed Changes

Copilot reviewed 135 out of 135 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
test/DocumentFormat.OpenXml.Linq.Tests/OpenXmlPartRootXElementExtensionsTests.cs	Removed "#nullable enable" directive and added null-forgiving operators on Document accesses.
test/DocumentFormat.OpenXml.Framework.Features.Tests/RandomParagraphIdGeneratorTests.cs	Updated null-forgiving operators on Document and Body accesses.
src/DocumentFormat.OpenXml/Packaging/WordprocessingDocument.cs	Added null-forgiving operator for Settings when appending the AttachedTemplate element.
samples/common/PowerPointUtils.cs	Introduced an explicit null check for Presentation in CreatePresentationParts.
samples/common/ExampleUtilities.cs	Added and adjusted null-checks and null-forgiving operators around core worksheet and workbook operations.
samples/Linq/SvgExample/StronglyTypedTools.cs	Added null checks for Presentation and Slide elements in SVG addition methods.
gen/DocumentFormat.OpenXml.Generator.Models/Generators/Parts/PartWriter.cs	Inserted a [DisallowNull] attribute and made the generated property return type nullable.

Comments suppressed due to low confidence (1)

test/DocumentFormat.OpenXml.Linq.Tests/OpenXmlPartRootXElementExtensionsTests.cs:11

Removal of '#nullable enable' may inadvertently disable nullable reference type checking in this file; consider reintroducing it if nullability enforcement is desired.

#nullable enable

Copilot · 2025-04-25T01:20:50Z


                        // Save the new worksheet.
-                        worksheetPart.Worksheet.Save();
+                        worksheetPart.Worksheet?.Save();


Using the null-conditional operator here may silently skip the Save() call if Worksheet is null; consider throwing an exception instead to maintain consistent failure behavior.

Suggested change

worksheetPart.Worksheet?.Save();

if (worksheetPart.Worksheet == null)

{

throw new InvalidOperationException("Worksheet is null. Unable to save changes.");

}

worksheetPart.Worksheet.Save();

Copilot · 2025-04-25T01:20:51Z

            writer.Write("public ");
            writer.Write(apiName);
-            writer.Write(" ");
+            writer.Write("? ");


Applying [DisallowNull] on a property whose type is rendered as nullable (using '?') is contradictory; please ensure that the nullability annotations and attributes are consistent.

Suggested change

writer.Write("? ");

writer.Write(" ");

It's not contradictory (there are times you may want this), but @tomjebo why do have both? This means it may be null, but a user can't set it to that. Do we want that?

@twsouthwick This was my misreading of your comment here in 959. I read that as [DisallowNull] and set the type to nullable with ?. I think you must have meant or but I was guessing there was a reason for wanting both. I'll take out the attribute [DisallowNull] as I think the root element property will have to be null at some point.

It also may have been early on in nullables and I misunderstood the point of it :) for now, just setting it to be nullable is good enough

so if the point is only to prevent assignment to null via static code analysis then I'm thinking we shouldn't use this as a way to enforce part root element minOccurs conformance. we should allow code to set to null (assuming eventually it will be set to a valid root element as required by the schemas) and then rely on our validation metadata to correctly catch the minOccurs constraint. that's my feeling. do you agree?

that sounds reasonable for now (that's the current behavior, right?)

mikeebowen · 2025-04-25T17:11:20Z


+            if (presentationPart.Presentation is null)
+            {
+                throw new ArgumentNullException(@"Presentation root element is missing!");


Nit, but you don't need the @ symbol when there are no characters to be escaped in a string.

I'll change both lines. I just copied that from above and didn't remove the superfluous @.

mikeebowen · 2025-04-25T17:11:45Z


+            if (slidePart.Slide is null)
+            {
+                throw new ArgumentNullException(@"Slide root element is missing!");


same nit as line 27

mikeebowen · 2025-04-25T17:11:52Z

        {
+            if (part.Presentation is null)
+            {
+                throw new ArgumentNullException(@"Presentation root element is missing!");


same nit as line 27

tomjebo · 2025-04-25T22:11:54Z

@twsouthwick @mikeebowen one more review if you have time. there was one more thing that I needed to add here and that was to enable nullability for projects that had any tests of the part root elements.

Therefore, I turned it on for DocumentFormat.OpenXml.Framework.Tests and DocumentFormat.OpenXml.Packaging.Tests. Then I fixed up the nullable violation warnings for these projects. DocumentFormat.OpenXml.Framework.Linq.Tests already had it on.

I initially turned it on for DocumentFormat.OpenXml.Tests but there were over 1700 violations and they aren't specifically testing root elements so I will defer that to a later migration.

Hopefully, now this satisfies #995 and #959.

twsouthwick · 2025-04-25T22:30:16Z

                .Elements<Presentation.SlideId>()
                .Select(slideId => (string)slideId.RelationshipId!)
-                .FirstOrDefault() ?? throw new InvalidOperationException(@"Presentation has no slides.");
+                .FirstOrDefault() ?? throw new InvalidOperationException("Presentation has no slides.");


if you want to ensure it's there, you can just call .First() and it will throw if it's not available

tomjebo added 2 commits April 24, 2025 17:48

make root element properties for derived parts to be nullable

b4c3bde

Merge remote-tracking branch 'upstream/main' into issue995

8d19536

tomjebo requested review from Copilot, mikeebowen and twsouthwick April 25, 2025 01:19

Copilot AI reviewed Apr 25, 2025

View reviewed changes

update utility to check for null

1b11370

mikeebowen previously approved these changes Apr 25, 2025

View reviewed changes

remove [DisallowNull]

81ac063

tomjebo dismissed mikeebowen’s stale review via 81ac063 April 25, 2025 18:24

twsouthwick previously approved these changes Apr 25, 2025

View reviewed changes

tomjebo requested a review from mikeebowen April 25, 2025 21:23

turn on nullable for projects that include part root element tests

eba7b64

tomjebo dismissed twsouthwick’s stale review via eba7b64 April 25, 2025 21:38

tomjebo requested a review from twsouthwick April 25, 2025 22:07

twsouthwick reviewed Apr 25, 2025

View reviewed changes

twsouthwick previously approved these changes Apr 25, 2025

View reviewed changes

use First instead of FirstOrDefault

10cbdbb

tomjebo dismissed twsouthwick’s stale review via 10cbdbb April 28, 2025 01:16

tomjebo added 2 commits April 28, 2025 08:15

revert to previous code

bc91d0a

Merge remote-tracking branch 'upstream/main' into issue995

4cd86d7

tomjebo enabled auto-merge (squash) April 28, 2025 15:54

mikeebowen approved these changes Apr 28, 2025

View reviewed changes

AlfredHellstern approved these changes Apr 28, 2025

View reviewed changes

tomjebo merged commit e069c61 into dotnet:main Apr 28, 2025
22 checks passed

mikeebowen requested a review from mkaszewiak April 28, 2025 16:04

Conversation

tomjebo commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomjebo Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomjebo commented Apr 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tomjebo commented Apr 25, 2025 •

edited

Loading

github-actions Bot commented Apr 25, 2025 •

edited

Loading

tomjebo Apr 25, 2025 •

edited

Loading