Investigate changing extensionlist/extension mechanism naming to more closely match Office use about open-xml-sdk

[tomjebo]

@twsouthwick Added unit test for API usage: tomjebo@51d8c74

[tomjebo]

@twsouthwick attempted code for a shim

Obsolete/Shim Temporary Approach

To avoid breaking the API and requiring a new major version, we were aiming to have something like the below. TableColumnExtensionList would return its own base ExtensionList. ExtensionList would operate as before except it would have the ObsoleteAttribute, letting developers know that they will need to stop using it at the next major version.

namespace DocumentFormat.OpenXml.Spreadsheet
{
    public partial class TableColumn : OpenXmlCompositeElement
    {
        /// <summary>
        /// <para>Gets or sets the legacy ExtensionList property.</para>
        /// <para>Future Extensibility.</para>
        /// <para>Represents the following element tag in the schema: x:extLst.</para>
        /// </summary>
        /// <remark>
        /// xmlns:x = http://schemas.openxmlformats.org/spreadsheetml/2006/main
        /// </remark>
        [ObsoleteAttribute("This property is obsolete and will be removed in a future version. Use the TableColumnExtension property instead.", false)]
        public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList
        {
            get => GetElement<DocumentFormat.OpenXml.Spreadsheet.ExtensionList>();
            set => SetElement((OpenXml.Spreadsheet.ExtensionList?)value);
        }

        /// <summary>Gets or sets the table column extension list.</summary>
        /// <value>The table column extension list.</value>
        public DocumentFormat.OpenXml.Spreadsheet.TableColumnExtensionList? TableColumnExtensionList
        {
            get => GetElement<DocumentFormat.OpenXml.Spreadsheet.ExtensionList>();
            set => SetElement((OpenXml.Spreadsheet.ExtensionList?)value);
        }
    }

    /// <summary>
    /// <para>Future Extensibility.</para>
    /// <para>This class is available in Office 2007 and above.</para>
    /// <para>When the object is serialized out as xml, it's qualified name is x:extLst.</para>
    /// </summary>
    /// <remark>
    /// <para>The following table lists the possible child types:</para>
    /// <list type="bullet">
    ///   <item><description><see cref="DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension" /> <c>&lt;x:ext></c></description></item>
    /// </list>
    /// </remark>
    public partial class TableColumnExtensionList : ExtensionList
    {
        public TableColumnExtensionList()
            : base()
        {
        }

        public TableColumnExtensionList(IEnumerable<OpenXmlElement> childElements)
            : base(childElements)
        {
        }

        public TableColumnExtensionList(params OpenXmlElement[] childElements)
            : base(childElements)
        {
        }

        public TableColumnExtensionList(string outerXml)
            : base(outerXml)
        {
        }

        internal override void ConfigureMetadata(ElementMetadata.Builder builder)
        {
            // base.ConfigureMetadata(builder);

            builder.SetSchema("x:extLst");
            builder.AddChild<DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension>();
            builder.Particle = new CompositeParticle.Builder(ParticleType.Sequence, 1, 1)
            {
                new ElementParticle(typeof(DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension), 0, 0),
            };
        }

        public override OpenXmlElement CloneNode(bool deep) => CloneImp<TableColumnExtensionList>(deep);
    }

    /// <summary>
    /// <para>Defines the TableColumnExtension Class.</para>
    /// <para>This class is available in Office 2007 and above.</para>
    /// <para>When the object is serialized out as xml, it's qualified name is x:ext.</para>
    /// </summary>
    /// <remark>
    /// <para>The following table lists the possible child types:</para>
    /// <list type="bullet">
    ///   <item><description><see cref="DocumentFormat.OpenXml.Office.SpreadSheetML.Y2023.MsForms.Question" /> <c>&lt;xlmsforms:question></c></description></item>
    /// </list>
    /// </remark>
    public partial class TableColumnExtension : OpenXmlCompositeElement
    {
        public TableColumnExtension()
            : base()
        {
        }

        public TableColumnExtension(IEnumerable<OpenXmlElement> childElements)
            : base(childElements)
        {
        }

        public TableColumnExtension(params OpenXmlElement[] childElements)
            : base(childElements)
        {
        }

        public TableColumnExtension(string outerXml)
            : base(outerXml)
        {
        }

        public StringValue? Uri
        {
            get => GetAttribute<StringValue>();
            set => SetAttribute(value);
        }

        internal override void ConfigureMetadata(ElementMetadata.Builder builder)
        {
            base.ConfigureMetadata(builder);
            builder.SetSchema("x:ext");
            builder.AddChild<DocumentFormat.OpenXml.Office.SpreadSheetML.Y2023.MsForms.Question>();
            builder.AddElement<TableColumnExtension>()
                .AddAttribute("uri", a => a.Uri, aBuilder =>
                {
                    aBuilder.AddValidator(RequiredValidator.Instance);
                    aBuilder.AddValidator(new StringValidator() { IsToken = (true) });
                });
            builder.Particle = new CompositeParticle.Builder(ParticleType.Choice, 1, 1)
            {
                new ElementParticle(typeof(DocumentFormat.OpenXml.Office.SpreadSheetML.Y2023.MsForms.Question), 1, 1, version: FileFormatVersions.Microsoft365),
                new AnyParticle(0, 1),
            };
        }

        /// <inheritdoc/>
        public override OpenXmlElement CloneNode(bool deep) => CloneImp<TableColumnExtension>(deep);
    }
}

Currently, even if we were able to get a shim like this to compile, the effect with our current particle/element constraints processing will not allow us to have choice between reading in and <extLst> element of type TableColumnExtensionList and ExtensionList.

[twsouthwick]

Can you add the existing pattern? nm I see you have that in there

[twsouthwick]

What about if you change the following:

public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList
{
  get => TableExtensionList;
  set => TableExtensionList = value;
} 

Of course with an appropriate conversion for the setter

[tomjebo]

@twsouthwick
Here's the test code we current are using. Of e1 and e2, one of them will always get null depending on the order of the AddChild's in the TableColumnExtensionList (see the third excerpt below).
For full changes, see https://github.com/tomjebo/Open-XML-SDK/tree/obsolete_extensions

    WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");
    if (sheet == null)
    {
        throw new ArgumentException();
    }

    TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");
    if (tblprt == null)
    {
        throw new ArgumentException();
    }

    TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();
    if (tc == null)
    {
        throw new ArgumentException("No columns found!");
    }

    DocumentFormat.OpenXml.Spreadsheet.Extension e1 = tc.ExtensionList.GetFirstChild<DocumentFormat.OpenXml.Spreadsheet.Extension>();
    DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension e2 = tc.TableColumnExtensionList.GetFirstChild<DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension>();

        [ObsoleteAttribute("This property is obsolete and will be removed in a future version. Use the TableColumnExtension property instead.", false)]
        public ExtensionList? ExtensionList
        {
            get => TableColumnExtensionList;
            set => TableColumnExtensionList = (TableColumnExtensionList?)value;
        }

        public TableColumnExtensionList? TableColumnExtensionList
        {
            get => GetElement<TableColumnExtensionList>();
            set => SetElement((TableColumnExtensionList?)value);
        }

Here are the AddChild methods that we would like to allow us to return either Extension or TableColumnExtension.
(We think the GetFirstChild would need to be modified to return either of these types for the same XML element i.e. <x:ext>)

    public partial class TableColumnExtensionList : ExtensionList
    {

...

        internal override void ConfigureMetadata(ElementMetadata.Builder builder)
        {
            // base.ConfigureMetadata(builder);

            builder.SetSchema("x:extLst");
            builder.AddChild<DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension>();
            builder.AddChild<DocumentFormat.OpenXml.Spreadsheet.Extension>();
            builder.Particle = new CompositeParticle.Builder(ParticleType.Sequence, 1, 1)
            {
                new ElementParticle(typeof(DocumentFormat.OpenXml.Spreadsheet.TableColumnExtension), 0, 0),
            };
        }

[twsouthwick]

You shouldn't need both in that location. Just have the specialized type. You're essentially proxying from the old type to the new type.

[mikeebowen]

We tried that. If we only have the specialized type then the child of the ExtensionList is a TableColumnExtension and if you try

DocumentFormat.OpenXml.Spreadsheet.Extension e1 = tc.ExtensionList.GetFirstChild<DocumentFormat.OpenXml.Spreadsheet.Extension>();

Then e1 i.e. the Extension is null.

[twsouthwick]

If we only have the specialized type then the child of the ExtensionList is a TableColumnExtension

Is that a problem? Given the details in the original message, it's the same type, just specialized now, right?

Do you have any unit tests to demonstrate the behaviors we want? That way we can play around with the implementation. (and of course they'll be failing for now)

Have you explored making use of the URI attribute? We should be able to parse that and surface that in the ExtensionList to know if it's the right thing.

[mikeebowen]

Is that a problem? Given the details in the original message, it's the same type, just specialized now, right?

Yes unfortunately, becuase if someone had used the original API they would have written GetFirstChild<DocumentFormat.OpenXml.Spreadsheet.Extension>();, because TableColumnExtension did not exist at the time.

Do you have any unit tests to demonstrate the behaviors we want? That way we can play around with the implementation. (and of course they'll be failing for now)

Not yet, but we know how we want the API to behave, so we should be able to add some.

Have you explored making use of the URI attribute? We should be able to parse that and surface that in the ExtensionList to know if it's the right thing.

The TableColumnExtension and Extension have the same URI, so this won't help distinguish between them.

[tomjebo]

Have you explored making use of the URI attribute? We should be able to parse that and surface that in the ExtensionList to know if it's the right thing.

We have not gone down this road in our implementation approaches yet, but definitely can. For this approach we wouldn't need the specialized extension list class i.e. TableColumnExtensionList. If we add this approach, then we could provide it as the sole solution for new extension XML elements like <x:tableColumn><xlmsforms:question>. But for all the existing extension elements that already also have specialized *ExtensionList classes, we would have to leave them untouched or add an ExtensionList property back into the parent (extended) element class to sit alongside the specialized *ExtentionList property. Not sure what all the ramifications of that would be but sounds feasible. At the least it could be somewhat confusing for developers until we do a major version where we just replace all the existing specialized extension list classes with this design.

[tomjebo]

@twsouthwick We did some brainstorming this morning and something like this could be an approach:

Whereas in the current API usage, we would have to do these:

#pragma warning disable CS0618 // Type or member is obsolete
                Extension e1 = tc.ExtensionList.GetFirstChild<Extension>();
#pragma warning restore CS0618 // Type or member is obsolete
                TableColumnExtension e2 = tc.TableColumnExtensionList.GetFirstChild<TableColumnExtension>();

Instead, if we add this kind of method to the ExtensionList class:

                Question q1 = tc.ExtensionList.GetChildExtension<Question>();

We could just avoid providing the specialized *ExtensionList and *Extension for the new extension element (i.e. <xlmsforms:question>. The GetChildExtension would use the type Question to get the uri guid associated with question and validate that it belongs to a TableColumn.ExtensionList.

Of course, we would then need to provide the code in our document consuming paths to validate XML blocks containing <question> under <extLst> and <ext>. So there would be a bit more work than just providing GetChildExtension but that would be the pattern for that scenario.

[twsouthwick]

That seems like a reasonable approach. Can you update what that means for the public API surface?

[tomjebo]

@twsouthwick TableColumnExtensionList is added as a shim and no longer a breaking change for the API with our test branch.
https://github.com/tomjebo/Open-XML-SDK/tree/obsolete_extensions

We have tests in TableColumnExtensionListTests.cs for this workaround.

[twsouthwick]

I couldn't find it in your branch (it's not searchable it seems). Can you provide a deep link? Or better update the API suggestion above with what it looks like?

[tomjebo]

The two test files that we currently have which show how the shims for TableColumn.ExtensionList and DifferentialType.ExtensionList will be used are here:

https://github.com/tomjebo/Open-XML-SDK/blob/obsolete_extensions/test/DocumentFormat.OpenXml.Framework.Tests/Particles/TableColumnExtensionListTests.cs

https://github.com/tomjebo/Open-XML-SDK/blob/obsolete_extensions/test/DocumentFormat.OpenXml.Framework.Tests/Particles/DxfExtensionListTests2006main.cs

I should clarify, that these do not represent the "new design" for the API but rather the obsoleted shim that will allow us to provide the new extension children until the next major release.

We will still need to iterate on the design going forward but this gets us past the breaking change for minor releases with new extension children.

[tomjebo]

@twsouthwick @mikeebowen

1a. Using TableColumn and the new Question element, here is the "diff" for the current schema update minor release that avoids breaking API changes:

namespace DocumentFormat.OpenXml.Spreadsheet
{
    public partial class TableColumn : OpenXmlCompositeElement
    {
+        [ObsoleteAttribute("This property is obsolete and will be removed in a future version. Use the TableColumnExtension property instead.", false)]
         public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList { get; set; }

+        public DocumentFormat.OpenXml.Spreadsheet.TableColumnExtensionList? TableColumnExtensionList { get; set; }
    }

With this change, here is the usage pattern for reading the XML:

...
            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

+#pragma warning disable CS0618 // Type or member is obsolete
+              Extension e = tc.ExtensionList.Descendants<Extension>().Where(e => e.Uri == "{FCC71383-01E1-4257-9335-427F07BE8D7F}").First();

+#pragma warning restore CS0618 // Type or member is obsolete

+              TableColumnExtension e2 = tc.TableColumnExtensionList.GetFirstChild<TableColumnExtension>();
+
+              Question question = e2.GetFirstChild<Question>();
+
+              question = e.GetFirstChild<Question>();
            }

With this change, here is the usage pattern for writing the XML:

            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

                ExtensionList eList = new ExtensionList(
                    new Extension() { Uri = "{FCC71383-01E1-4257-9335-427F07BE8D7F}" });

                tc.AddChild(eList);

                // Or

+#pragma warning disable CS0618 // Type or member is obsolete
                tc.ExtensionList = eList;
+#pragma warning restore CS0618 // Type or member is obsolete

+                TableColumnExtensionList teList = new TableColumnExtensionList(
+                   new TableColumnExtension(
+                       new Question() { Id = "r8a22544ad01d478e898ac5748745f765" })
+                    { Uri = "{FCC71383-01E1-4257-9335-427F07BE8D7F}" });
+
+                tc.AddChild(teList);
+
+                // Or 
+
+                tc.TableColumnExtensionList = teList;
       }

1b. Using TableColumn and the new Question element, here is the "diff" when a major version allows breaking API changes and the above obsolete shim is removed:

namespace DocumentFormat.OpenXml.Spreadsheet
{
    public partial class TableColumn : OpenXmlCompositeElement
    {
-        [ObsoleteAttribute("This property is obsolete and will be removed in a future version. Use the TableColumnExtension property instead.", false)]
-        public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList { get; set; }

+       public DocumentFormat.OpenXml.Spreadsheet.TableColumnExtensionList? TableColumnExtensionList { get; set; }
    }

With this change, here is the usage pattern for reading the XML:

...
            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

-#pragma warning disable CS0618 // Type or member is obsolete
-              Extension e = tc.ExtensionList.Descendants<Extension>().Where(e => e.Uri == "{FCC71383-01E1-4257-9335-427F07BE8D7F}").First();
-#pragma warning restore CS0618 // Type or member is obsolete

+              TableColumnExtension e2 = tc.TableColumnExtensionList.GetFirstChild<TableColumnExtension>();
+
+              Question question = e2.GetFirstChild<Question>();
+
-              question = e.GetFirstChild<Question>();
            }

With this change, here is the usage pattern for writing the XML:

            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

-                ExtensionList eList = new ExtensionList(
-                    new Extension() { Uri = "{FCC71383-01E1-4257-9335-427F07BE8D7F}" });

-                tc.AddChild(eList);

-                // Or

-#pragma warning disable CS0618 // Type or member is obsolete
-                tc.ExtensionList = eList;
-#pragma warning restore CS0618 // Type or member is obsolete

+                TableColumnExtensionList teList = new TableColumnExtensionList(
+                   new TableColumnExtension(
+                       new Question() { Id = "r8a22544ad01d478e898ac5748745f765" })
+                    { Uri = "{FCC71383-01E1-4257-9335-427F07BE8D7F}" });
+
+                tc.AddChild(teList);
+
+                // Or 
+
+                tc.TableColumnExtensionList = teList;
       }

2. Here is a proposed "diff" if for a new design that operates like Office, i.e. runtime comparison of Uri for an extension child.

namespace DocumentFormat.OpenXml.Spreadsheet
{
    public partial class TableColumn : OpenXmlCompositeElement
    {
         public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList { get; set; }
    }

With this change, here is the usage pattern for reading the XML:

...
            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

                Extension e = tc.ExtensionList.Descendants<Extension>().Where(e => e.Uri == "{FCC71383-01E1-4257-9335-427F07BE8D7F}").First();

+              Question question = e.GetFirstChild<Question>();
+
+              // Or alternatively, a new method that encapsulates everthing including tc.ExtensionList.Descendants and Question:
+
+              Question question = tc.GetExtensionChild<Question>();
            }

With this change, here is the usage pattern for writing the XML:

            using (SpreadsheetDocument spd = SpreadsheetDocument.Open(stream, false))
            {
                WorksheetPart sheet = (WorksheetPart)spd.WorkbookPart.GetPartById("rId1");

                TableDefinitionPart tblprt = (TableDefinitionPart)sheet.GetPartById("rId2");

                TableColumn tc = tblprt.Table.TableColumns.GetFirstChild<TableColumn>();

                ExtensionList eList = new ExtensionList(
                    new Extension(
+                       new Question() { Id = "r8a22544ad01d478e898ac5748745f765" })
                       { Uri = "{FCC71383-01E1-4257-9335-427F07BE8D7F}" });

                tc.AddChild(eList);

                // Or

                tc.ExtensionList = eList;
+
+              // Or alternatively, a new method that encapsulates everything including new ExtensionList, AddChild, etc...:
+
+              Question question = tc.AddExtensionChild<Question>();
       }

For classes which contained previously generated specialized extensionlist properties

The API definition would switch from using (for example) TableExtensionList to just ExtensionList. To update code to this would be a breaking change (i.e. removing TableExtensionList/TableExtention):

   public partial class Table : OpenXmlPartRootElement
   {
-         public DocumentFormat.OpenXml.Spreadsheet.TableExtensionList? TableExtensionList { get; set; }
+        public DocumentFormat.OpenXml.Spreadsheet.ExtensionList? ExtensionList  { get; set; }
   }

Obviously we could use a shim for minor version releases to migrate folks.

3. Here is a proposed "diff" if for a new design that is similar to option 2 above but we don't add validation based on Uri values.

So the code diff would look essentially the same as option 2 but runtime doesn't check any validation under Extension. The reason for this is that ExtensionList/Extension actually do not have any restrictions for child elements in the ISO standard or in the Office behavior.

[twsouthwick]

OK - sounds like we'll continue with the pattern we had, but just not remove the extension and obsolete it?

[mikeebowen]

OK - sounds like we'll continue with the pattern we had, but just not remove the extension and obsolete it?

Yes, maybe. That is option 1 and what is currently in PR #1708. 2 & 3 are other options for the API.

IMO Option 2 seems like a lot of work for no or very little benefit.

Continuing with option 1 and obsoleting ExtensionList and Extension until we hit a major release to remove them would allow us to continue with the same pattern to create FeaturExtensions and restrict their children.

But this is not actually what the standard says. According to ISO 2016-1 18.2.7 ext (Extension), any valid xml element can be a child of Extension:

Upon encountering extensions, a processing consumer shall determine whether it knows how to process extensions using the value of the uri. If the consumer knows how to process such an extension, the markup contained within that extension is processed. Otherwise, the extension content shall be preserved so long as the structure that contains the extLst has not been removed.

and the schema for CT_Extension is

<xsd:complexType name="CT_Extension">
  <xsd:sequence>
    <xsd:any processContents="lax"/>
  </xsd:sequence>
  <xsd:attribute name="uri" type="xsd:token"/>
</xsd:complexType>

If we go with option 3, we lose the child validation, but we won't have to make a specialized extension manually for every use of ExtenstionList and Extension and if we want to mirror the standard this is actually correct. It should also allow us to clean up a lot of code and avoid introducing bugs by manually patching.