Remove erroneous curlies in Xml resource

[mrj001]

This is addressing one of the 13 strings in issue 30218 that have faulty formatters (Sch_MinLengthGtBaseMinLength). I'd like some feedback on this approach before I tackle the other 12.

[dnfclas]

All CLA requirements met.

[danmoseley]

Looks good but I would either change the test to be locale-independent, or (if there's already functional coverage) consider not bothering with a test at all. This would be acceptable for any cases where you are literally just changing the text of the string and there are no replacement parameters in it.

[danmoseley]

Looks like test failures are unrelated - if you push your edit, it will run again.

BTW, we prefer to set the PR title to something that mentions to issue, not a number.

[mrj001]

@danmosemsft
Sorry it took so long to get back to this. I was blocked on trying to generate the code coverage report. Finally gave up and filed #34152 . How does one determine the code coverage with coverlet?

Since I don't have this information, I've decided to proceed with unit tests. I suspect that if unit tests did cover these strings, they would have had proper formatters in them. Also, it confirms to me that I understand exactly when these are being used.

In looking at Sch_LengthGtBaseLength, I have come up with two questions. This resource is only used in method CompileLengthFacet in file FacetChecker.cs:

                if ((_baseFixedFlags & RestrictionFlags.Length) != 0)
                {    
                    if (!_datatype.IsEqual(_datatype.Restriction.Length, _derivedRestriction.Length))
                    {    
                        throw new XmlSchemaException(SR.Sch_FacetBaseFixed, facet);
                    }    
                }    
                if ((_baseFlags & RestrictionFlags.Length) != 0)
                {    
                    if (_datatype.Restriction.Length < _derivedRestriction.Length)
                    {    
                        throw new XmlSchemaException(SR.Sch_LengthGtBaseLength, facet);
                    }    
                }    

I can trigger the exception to throw with the message SR.Sch_LengthGtBaseLength by specifying a length facet in both a base type and a derived type, with the derived type having a longer length.

Q1: Isn't the length facet supposed to specify the exact length? If so, why is the second test above checking for "less than" rather than "not equal"?
Q2: what is _baseFixedFlags vs. _baseFlags? The first test was not triggered by my test case, but the second one was. I'm curious as to why I'm not triggering the first test.

This is the test schema that I use to provoke the exception.

<?xml version='1.0' encoding='utf-8' ?>
<xs:schema elementFormDefault='qualified'
           xmlns:xs='http://www.w3.org/2001/XMLSchema'>
    <xs:simpleType name='foo'>
        <xs:restriction base='xs:string'>
            <xs:length value='5'/>
        </xs:restriction>
    </xs:simpleType>
    <xs:simpleType name='bar'>
        <xs:restriction base='foo'>
            <xs:length value='6'/>
        </xs:restriction>
    </xs:simpleType>
</xs:schema>

[danmoseley]

@mrj001 are the instructions in https://github.com/dotnet/runtime/blob/master/docs/workflow/building/libraries/code-coverage.md not working? If not @ViktorHofer owns them.

For your other questions, @buyaa-n owns System.Xml and can help.

[buyaa-n]

Q1: Isn't the length facet supposed to specify the exact length?

Yes, but only for that type i assume (in this case the base type)

If so, why is the second test above checking for "less than" rather than "not equal"?

Because derived type can have length less than or equal to base type length, why do you think it must be equal?

Q2: what is _baseFixedFlags vs. _baseFlags?

Looks _baseFixedFlags is for only inferring if base type is fixed (which can be a boolean), but _baseFlags has RestrictionFlags of base (Length, MinLength, MaxLength ... etc)

The first test was not triggered by my test case, but the second one was. I'm curious as to why I'm not triggering the first test.

I assume you need to set fixed value for base type definition for example:
<xs:simpleType name='foo' type="xs:string" fixed="FixedValue">

[mrj001]

@buyaa-n

Because derived type can have length less than or equal to base type length, why do you think it must be equal?

I initially relied on this page's description:
https://www.w3schools.com/xml/schema_facets.asp
A derived type would fail the "Is A" test if its length were different from that of its base class.

For greater surety, see the note in paragraph 4.3.1.1 of:
https://www.w3.org/TR/2012/REC-xmlschema11-2-20120405/datatypes.html

In Paragraph 4.3.1.4 of the same link:

Schema Component Constraint: length valid restriction
It is an ·error· if length is among the members of {facets} of {base type definition} and {value} is not equal to the {value} of the parent length.

And a unit test (failing) to demonstrate. Shall I file a separate issue? This is getting off topic of this pull request.

        [Fact]
        public void LengthLtBaseLength_Throws()
        {
            string schema = @"<?xml version='1.0' encoding='utf-8' ?>
<xs:schema elementFormDefault='qualified'
           xmlns:xs='http://www.w3.org/2001/XMLSchema'>
    <xs:simpleType name='foo'>
        <xs:restriction base='xs:string'>
            <xs:length value='7'/>
        </xs:restriction>
    </xs:simpleType>
    <xs:simpleType name='bar'>
        <xs:restriction base='foo'>
            <xs:length value='6'/>
        </xs:restriction>
    </xs:simpleType>
</xs:schema>
";

            XmlSchemaSet ss = new XmlSchemaSet();
            ss.Add(null, XmlReader.Create(new StringReader(schema)));

            Exception ex = Assert.Throws<XmlSchemaException>(() => ss.Compile());
            Assert.Contains("length", ex.Message);
        }

editted to fix hyperlinks.

[buyaa-n]

Schema Component Constraint: length valid restriction
It is an ·error· if length is among the members of {facets} of {base type definition} and {value} is not equal to the {value} of the parent length.

I think that is valid only for fixed length:

That is why we have this condition:

if ((_baseFixedFlags & RestrictionFlags.Length) != 0)
{    
    if (!_datatype.IsEqual(_datatype.Restriction.Length, _derivedRestriction.Length))
    {    
        throw new XmlSchemaException(SR.Sch_FacetBaseFixed, facet);
    }    
} 

[buyaa-n]

And a unit test (failing) to demonstrate. Shall I file a separate issue? This is getting off topic of this pull request.

By counting my above comment your test should fail unless it has fixed = true attibute. So this is valid scenario, no need to file issue

[mrj001]

@buyaa-n
This is the note from Paragraph 4.3.1.1 (emphasis added):

Note: The {fixed} property is defined for parallelism with other facets and for compatibility with version 1.0 of this specification. But it is a consequence of length valid restriction (§4.3.1.4) that the value of the length facet cannot be changed, regardless of whether {fixed} is true or false.

[buyaa-n]

Note: The {fixed} property is defined for parallelism with other facets and for compatibility with version 1.0 of this specification. But it is a consequence of length valid restriction (§4.3.1.4) that the value of the length facet cannot be changed, regardless of whether {fixed} is true or false.

@mrj001 that is good point, I understand and really appreciate your effort for trying to make this right, but FYI we only support version 1.0

[buyaa-n]

Looks good but I would either change the test to be locale-independent, or (if there's already functional coverage) consider not bothering with a test at all. This would be acceptable for any cases where you are literally just changing the text of the string and there are no replacement parameters in it.

@danmosemsft I think @mrj001 addressed your change request, tests added using schema attribute names instead of entire error messages as string literal, tests looks good and improving code coverage, do you have any other comments/suggestions?

Tagging also @krwq as he is back

[danmoseley]

@buyaa-n don't wait on me, I trust your review

[buyaa-n]

@buyaa-n don't wait on me, I trust your review

Thanks, as you have requested change we still need your approval 😄, the merge button still disabled for me

[danmoseley]

Oops, done

[danmoseley]

Thank you @mrj001 for the contribution. Perhaps you'd be interested in looking at another issue that is labeled up for grabs.

[krwq]

LGTM post merge