Skip to content
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

new CellValue(bool) generates invalid XML #1070

Closed
stevehansen opened this issue Nov 16, 2021 · 4 comments
Closed

new CellValue(bool) generates invalid XML #1070

stevehansen opened this issue Nov 16, 2021 · 4 comments

Comments

@stevehansen
Copy link
Contributor

Description

The introduced new CellValue(bool) uses the wrong serialization causing Excel to fail to load the file.

Information

Repro

Debug.Assert(new CellValue(true).InnerText == "true");

Observed

Spec expects the boolean values to be "true" or "false" but the code is using bool.ToString() which returns "True" or "False".

https://github.com/OfficeDev/Open-XML-SDK/blob/main/src/DocumentFormat.OpenXml/Spreadsheet/CellValue.cs#L42

Expected

Per .NET remarks for the boolean.ToString:

This method returns the constants "True" or "False".

Note that XML is case-sensitive, and that the XML specification recognizes "true" and "false" as the valid set of Boolean values. If the string returned by the ToString() method is to be written to an XML file, its String.ToLowerInvariant method should be called first to convert it to lowercase.

@stevehansen
Copy link
Contributor Author

The validator uses the .NET bool.TryParse which allows both "True" and "true" so isn't exactly correct.
CellTests is using the string "true" and "false" directly and didn't use the new constructor.
SpreadsheetCellTests has no tests for boolean.

stevehansen added a commit to stevehansen/Open-XML-SDK that referenced this issue Nov 17, 2021
@twsouthwick
Copy link
Member

@tomjebo can you verify this from the spec side? I'm good taking a fix for this, just want to verify the spec

@twsouthwick
Copy link
Member

@tomjebo I merged this in. Please let me know if there's any issue with the spec.

@tomjebo
Copy link
Collaborator

tomjebo commented Dec 2, 2021

@twsouthwick @stevehansen thanks for getting this in. Our implementer notes and 29500 are not explicit on the case.
MS-OI29500 2.1.659 says:

If the cell contains a Boolean, the value shall be 1 to specify true and 0 to specify false.

The W3C spec has this:

[Definition:]  boolean has the ·value space· required to support the mathematical concept of binary-valued logic: {true, false}.

3.2.2.1 Lexical representation
An instance of a datatype that is defined as ·boolean· can have the following legal literals {true, false, 1, 0}.

3.2.2.2 Canonical representation
The canonical representation for boolean is the set of literals {true, false}.

3.2.2.3 Constraining facets
boolean has the following ·constraining facets·:

pattern
whiteSpace

Note the "legal literals".
And Excel does balk at "True" and "False" so IMO this fix is warranted.

@tomjebo tomjebo closed this as completed Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants