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

Xml resources have invalid formatter #30218

Closed
krwq opened this issue Jul 11, 2019 · 7 comments · Fixed by #35013
Closed

Xml resources have invalid formatter #30218

krwq opened this issue Jul 11, 2019 · 7 comments · Fixed by #35013
Assignees
Labels
area-System.Xml bug good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@krwq
Copy link
Member

krwq commented Jul 11, 2019

i.e. https://github.com/dotnet/corefx/blob/master/src/System.Private.Xml/src/Resources/Strings.resx#L1722

{facets} instead of i.e. {0}. This is likely causing some random exceptions to some people

@am11
Copy link
Member

am11 commented Jul 11, 2019

Seems like there are 13 invalid cases:

Sch_InvalidAllMax: 'all' must have {max occurs}=1.
Sch_InvalidAllElementMax: The {max occurs} of all the particles in the {particles} of an all group must be 0 or 1.
Sch_InvalidExamplar: Cannot be nominated as the {substitution group affiliation} of any other declaration.
Sch_GroupBaseRestNotEmptiable: All particles in the {particles} of the base particle which are not mapped to by any particle in the {particles} of the derived particle should be emptiable - All:All,Sequence:Sequence -- Recurse Rule 2 / Choice:Choice -- RecurseLax.
Sch_AllRefMinMax: The group ref to 'all' must have {min occurs}= 0 or 1 and {max occurs}=1.
Sch_AttributeRestrictionInvalidFromWildcard: The {base type definition} must have an {attribute wildcard} and the {target namespace} of the R's {attribute declaration} must be valid with respect to that wildcard.
Sch_LengthGtBaseLength: It is an error if 'length' is among the members of {facets} of {base type definition} and {value} is greater than the {value} of the parent 'length'.
Sch_MinLengthGtBaseMinLength: It is an error if 'minLength' is among the members of {facets} of {base type definition} and {value} is less than the {value} of the parent 'minLength'.
Sch_MaxLengthGtBaseMaxLength: It is an error if 'maxLength' is among the members of {facets} of {base type definition} and {value} is greater than the {value} of the parent 'maxLength'.
Sch_MaxMinLengthBaseLength: It is an error for both 'length' and either 'minLength' or 'maxLength' to be members of {facets}, unless they are specified in different derivation steps. In which case the following must be true: the {value} of 'minLength' <= the {value} of 'length' <= the {value} of 'maxLength'.
Sch_FacetBaseFixed: Values that are declared as {fixed} in a base type can not be changed in a derived type.
Sch_WhiteSpaceRestriction1: It is an error if 'whiteSpace' is among the members of {facets} of {base type definition}, {value} is 'replace' or 'preserve', and the {value} of the parent 'whiteSpace' is 'collapse'.
Sch_WhiteSpaceRestriction2: It is an error if 'whiteSpace' is among the members of {facets} of {base type definition}, {value} is 'preserve', and the {value} of the parent 'whiteSpace' is 'replace'."

found by navigating to raw view https://raw.githubusercontent.com/dotnet/corefx/master/src/System.Private.Xml/src/Resources/Strings.resx and in the Firefox's console, running:

// edit: works in other new-gen browsers
[...  // spread operator to convert NodeList to Array object
  new DOMParser()
  .parseFromString(
    document
      .querySelector('body > pre').innerText, 'text/xml')
      .documentElement.childNodes
].filter(node => node.querySelector && node.querySelector('data > value') &&
    /\{(\d*[a-zA-Z\s]+\d*)+\}/.test(node.querySelector('data > value').textContent))
 .map(node =>
    `${node.getAttribute('name')}: ${node.querySelector('data > value').textContent}`)
 .join('\n')

@krwq
Copy link
Member Author

krwq commented Jul 11, 2019

@am11 I've done it with just regex: \{[^\d] but that works too

@krwq krwq self-assigned this Jul 11, 2019
@am11
Copy link
Member

am11 commented Jul 11, 2019

It was just to capture mixed cases like {123abc}, {abc123}, {123 abc456 xyz} etc.

@danmoseley
Copy link
Member

String.Format throws FormatException if there are more replacement markers than supplied replacements, but does not and has never thrown if there are fewer markers. (For some reason)

So if some {0} etc are missing, I would not expect String.Format to throw.

In other projects, I've sometimes had a Debug.Assert at the point we do the String.Format that asserts that there are not fewer replacement markers than replacements. Assuming we fix this and are otherwise consistent, that might be worth considering. Looks like in this case it's maybe the one in XmlException.CreateMessage().

@danmoseley
Copy link
Member

danmoseley commented Jul 11, 2019

BTW, I see these strings look the same in .NET Framework. I do not believe this meets the bar for 3.0 at this point (assuming it's even a bug and it isn't somehow meant to be this way)

@krwq krwq removed their assignment Jul 12, 2019
@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the Future milestone Feb 1, 2020
@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Feb 23, 2020
@danmoseley danmoseley added help wanted [up-for-grabs] Good issue for external contributors good first issue Issue should be easy to implement, good for first-time contributors labels Feb 24, 2020
@mrj001
Copy link
Contributor

mrj001 commented Mar 20, 2020

I'd like to tackle this as a first contribution. I was not able to find any way to claim this issue. I think I need to be added for this to be assigned to me.

I've created pull request #33890 for one of the 13 resources for feedback before tackling all of them.

@danmoseley
Copy link
Member

Hi @mrj001 glad to have you on board. I assigned you this issue.

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Mar 31, 2020
@buyaa-n buyaa-n modified the milestones: Future, 5.0 Mar 31, 2020
danmoseley pushed a commit to danmoseley/runtime that referenced this issue Apr 3, 2020
danmoseley pushed a commit that referenced this issue Apr 18, 2020
* Added test for Issue 30218, use of resource Sch_MinLengthGtBaseMinLength

* Reworded error message to remove the invalid formatters.

Issue #30218

* Updated check of message from the exception to be more amenable to potential internationalization.

* Reworded resource Sch_MaxLengthGtBaseMaxLength and added unit test for same.

* Simplified test code per feedback from Dan Moseley, PR#33890.

* Tests to increase code coverage.   The ones that are failing trigger resource string Sch_MaxMinLengthBaseLength to be used as the Exception message.

* Removed invalid formatters from the resource string Sch_MaxMinLengthBaseLength.  Failing tests from the previous commit now pass.

* Added test for the Exception using string resource Sch_LengthGtBaseLength as its message.  This covers a source line and branch not previously covered.  This test fails due to the invalid formatters.

* Reworded string resource Sch_LengthGtBaseLength to remove the invalid formatters.

* Added unit tests for uses of string resource Sch_FacetBaseFixed.

* Made test for the message containing "fixed" more specific.

* Reworded string resource Sch_FacetBaseFixed to remove the invalid formatter.

* Added unit test to cover usage of string resource Sch_InvalidAllMax.  A second usage of this string resource is not covered as it is in a class marked Obsolete.

* Reworded string resource Sch_InvalidAllMax to remove the invalid formatter.

* Added unit test for String Resource Sch_InvalidAllElementMax - invalid formatter.

* Reworded string resource Sch_InvalidAllElementMax to remove the invalid formatter.

* Added unit test which causes an XmlSchemaException using string resource Sch_InvalidExemplar as its message.

* Reworded Sch_InvalidExemplar to remove the invalid formatter.  Also, added the name of the element which cannot be used as the substitution group affiliation.

* Added unit test that causes an XmlSchemaException to be thrown using string resource Sch_GroupBaseRestNotEmptiable as its message.

* Removed the curly braces so there are no longer any invalid formatters.

* Added unit tests that cause an XmlSchemaException to be thrown using string resource Sch_AllRefMinMax as its message.

* Changed string resource Sch_AllRefMinMax to remove the invalid formatters.

* reworded Sch_MinLengthGtBaseMinLength and Sch_MaxLengthGtBaseMaxLength similarly to Sch_LengthGtBaseLength

* Corrected error "greater than" to "less than".

* Changed name of test to more accurately  reflect its purpose.

* Changed casing of MinLength and MaxLength to match their XML facets.  Removed redundant ToLower call and comment.

* Removed all Regex used to find invalid formatters.

* renamed MaxMinLengthBaseLength_TestData to indicate that this is testing the successful case.

* Removed suppression of exception so that we will see the exception if one is thrown.

* Removed comments as there are issues tracking these.

* Fixed comment larger -> lower.

* Removed XML comments on test methods.

* Removal of XML comment that was missed.
@ghost ghost locked as resolved and limited conversation to collaborators Dec 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Xml bug good first issue Issue should be easy to implement, good for first-time contributors help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants