Fix Abort when documentPart has no type

[olabusayoT]

• add TDMLException when there is no type
• add tests

DAFFODIL-561

[mbeckerle]

+1

[stevedlawrence]

+1

The change looks fine, but do we really need this check? Feels like we already handle this except in cases where maybe it doesn't matter?

[stevedlawrence]

I'm surprised we need this, the tdml.xsd file defines this as documentPart:

  <element name="documentPart" type="tns:documentPartType"/>

  <complexType name="documentPartType">
    <simpleContent>
      <extension base="xs:string">
        <attribute name="type" type="tns:documentPartTypeEnum" use="required"/>
        ...
      </extension>
    </simpleContent>
  </complexType>

  <simpleType name="documentPartTypeEnum">
    <restriction base="xs:string">
      <enumeration value="byte"/>
      <enumeration value="text"/>
      <enumeration value="bits"/>
      <enumeration value="file"/>
    </restriction>
  </simpleType>

Seems like this already requires the type attribute and it must be one of byte/text/bits/file. So I would think TDML XSD validation should catch this? This ticket was opened in 2013, maybe we didn't do XSD validation back then? I guess some unit tests could disable/skip validation, but then I would think they would expect these kinds of failures? Do we really want to reimplement XSD validation in scala just in case a unit tests disables it?

[olabusayoT]

Good point...we only run into the assertion when call the Document method directly! I guess I wonder if that is a valid use case of the TDMLRunner? If it is, that bypasses the tdml.xsd and I think we ought to add in the empty case check, but if that's not a valid usecase, then I agree this PR isn't really necessary.

Per the ticket, we no longer get an Abort. But we do get an invariantFailed error when calling the Document method with the invalid tdml.

[stevedlawrence]

I would say the only valid use case of using non-validated TDML is for unit tests, in which case I'm less worried about these kinds of issues, since presumably unit tests are starting with valid TDML.

As long as we get a reasonable diagnostic when using the Runner class with invalid TDML I'd prefer to avoid adding additional checks to keep the TDML Runner as simple as possible. We should probably only include checks that XSD validation can't support.

[mbeckerle]

I think it is not a supported use case for the TDML runner to NOT validate the incoming TDML XML.

Why do we need that even in unit tests? My guess is such tests no longer add value and we should remove them.

Code should not redundantly check things that are assured by XML schema validity, as it creates confusion about what needs to be checked and what does not.

[olabusayoT]

Just to check I'm reading that right, the TDML Runner should be validating incoming TDML? i.e when we do Runner(xml) it should validate the incoming xml. I believe it currently does that.

I updated the code to remove the scala check, but I left in one of the tests that ensures XML schema vailidity so we could point to that when we closed the ticket