-
Notifications
You must be signed in to change notification settings - Fork 551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Investigate changing extensionlist/extension mechanism naming to more closely match Office use #1697
Comments
@twsouthwick attempted code for a shim Obsolete/Shim Temporary ApproachTo avoid breaking the API and requiring a new major version, we were aiming to have something like the below. 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><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><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 |
|
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 |
@twsouthwick 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. 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),
};
} |
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. |
We tried that. If we only have the specialized type then the child of the DocumentFormat.OpenXml.Spreadsheet.Extension e1 = tc.ExtensionList.GetFirstChild<DocumentFormat.OpenXml.Spreadsheet.Extension>(); Then |
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. |
Yes unfortunately, becuase if someone had used the original API they would have written
Not yet, but we know how we want the API to behave, so we should be able to add some.
The |
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. |
@twsouthwick Added unit test for API usage: tomjebo@51d8c74 |
@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 Of course, we would then need to provide the code in our document consuming paths to validate XML blocks containing |
That seems like a reasonable approach. Can you update what that means for the public API surface? |
@twsouthwick TableColumnExtensionList is added as a shim and no longer a breaking change for the API with our test branch. We have tests in TableColumnExtensionListTests.cs for this workaround. |
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? |
The two test files that we currently have which show how the shims for TableColumn.ExtensionList and DifferentialType.ExtensionList will be used are here: 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. |
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 propertiesThe 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. |
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 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
and the schema for <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 |
History
Office team provides CT_ExtensionList and CT_Extension types in xsd definitions and uses a uri attribute in the new child elements under CT_Extension to authorize the relationship between the child and parent.
In the backend of the SDK (not seen to the public), we have been using specialized CT_<feature>ExtensionList/CT_<feature>Extension complex types to generate C# classes which emit the same structure as above but provide this parent/child relationship authorization at compile time with strong typing instead of runtime via guids.
NOTE: here refers to the parent type/class feature that uses the extension children being added.
By creating specialized CT_<feature>ExtensionList/CT_<feature>Extension derived C# classes, we created mutual exclusivity between the CT_Extension* derived properties that exist before extending the parent type and the new specialized CT_<feature>Extenstion* properties that we build after Office extends the parent type.
The exclusivity, comes from the fact that both the CT_Extension* and CT_<feature>Extenstion* represent the same element as derived from OpenXmlCompositeElement. Because of this exclusivity, when we release a new child extension, we replace the existing CT_Extension* property in the parent with the new CT_<feature>Extension* property.
This replacement sometimes represents a breaking change in the SDK framework API. In order to maintain semantic versioning we would have to maintain both CT_Extension* and CT_<feature>Extenstion* properties in the extended parent type and create a shim or migration pathway from the old to the new (until a major version).
Problem Statement
Avoid introducing a breaking change.
To qualify this, only when we release a new child extension to a type that has never had a child extension added by Office.
Approaches
One-time Refactor
One approach to this would be to replace all existing references to CT_ExtensionList and CT_Extension in the base Office xsds used by the SDK with CT_<feature>ExtensionList and CT_<feature>Extension appropriate to their use in the parent types. This would provide the extension list structure to allow new children extension elements to be added without breaking the SDK API going forward. This approach would require a one-time breaking change across the SDK but would avoid breaking changes going forward. If Office introduces a new parent type or adds a parent type with a CT_ExtensionList property, we could intercept that before it releases as an SDK class and change it to the CT_<feature>ExtensionList model.
Redesign To Use CT_Extension*
This involves redesigning the SDK validation, particle and element production and consumption processing code to use the Office method of Uri guids to determine parent/extension child relationships at runtime and move away from strong type enforcement.
Other <TBD>
The text was updated successfully, but these errors were encountered: