Parser treats missing required elements inconsistently

[scpeters]

First noticed in #389 (comment):

Do required fields need defaults? When would the default be used?

After writing that comment, I saw below that leaving this element out is supposed to still go through, but generates an error... I haven't looked, but is this kind of behaviour documented?

In researching how to respond to this comment I found some inconsistent code (code links from 10.0.0)

• Near the beginning of readXml() in parser.cc, there is a block of code that looks like it fails hard whenever a required element is missing

• Later in the function (parser.cc:1131-1156), there is code that generates errors if any required child elements are missing, but only for joints that aren't of type ball! This seems to apply to //joint/parent and //joint/child. But for anything else, required elements are simply inserted with default values and no error message.

I'll try to make examples that illustrate the different types of behavior here.

[scpeters]

• Near the beginning of readXml() in parser.cc, there is a block of code that looks like it fails hard whenever a required element is missing

this check has no test coverage as of c1e703d. it may be dead code

[scpeters]

I've added some example files with missing //joint/child elements in scpeters@3e7f475.

fixed_joint_child_missing.sdf has a hard failure when trying to print it:

$ ign sdf --print fixed_joint_child_missing.sdf 
Error Code 7 Msg: XML Missing required element[child], child of element[joint]
Error Code 8 Msg: Error reading element <joint>
Error Code 8 Msg: Error reading element <model>
Error Code 8 Msg: Error reading element <sdf>
Error: SDF parsing the xml failed.

ball_joint_child_missing.sdf is able to print successfully, with the //joint/child element populated by the default value __default__:

$ ign sdf --print ball_joint_child_missing.sdf 
<sdf version='1.7'>
  <model name='ball_joint_child_missing'>
    <link name='link'>
      <pose>0 0 1 0 -0 0</pose>
    </link>
    <joint name='joint' type='ball'>
      <pose>0 0 3 0 -0 0</pose>
      <parent>link</parent>
      <child>__default__</child>
    </joint>
  </model>
</sdf>

This is an invalid value, though, so ign sdf --check fails:

$ ign sdf --check ball_joint_child_missing.sdf 
Error: Child link with name[__default__] specified by joint with name[joint] not found in model with name[ball_joint_child_missing].
Error: FrameAttachedToGraph error, Non-LINK vertex with name [joint] is disconnected; it should have 1 outgoing edge in MODEL attached_to graph.
Error: Graph has __model__ scope but sink vertex named [joint] does not have FrameType LINK OR MODEL when starting from vertex with name [joint].
Error: Child link with name[__default__] specified by joint with name[joint] not found in model with name[ball_joint_child_missing].
Error: PoseRelativeToGraph error, Vertex with name [joint] is disconnected; it should have 1 incoming edge in MODEL relative_to graph.
Error: PoseRelativeToGraph frame with name [joint] is disconnected; its source vertex has name [joint], but its source name should be __model__.

[scpeters]

The required attribute in the specification maps to two different behaviors that I can reproduce:

1. Hard parsing failure if //joint/parent or //joint/child are missing and //joint/@type != ball
2. Silently populating the missing field with a default value if missing.

If we want the parser to perform both of these behaviors, I would propose creating a new attribute like autofill_if_missing (but with a better name) and update the spec accordingly. Currently I believe this is why certain fields get added to an SDFormat file when you print it even if they weren't in the original document.

[chapulina]

I would propose creating a new attribute like autofill_if_missing

Would this be an attribute that the person writing the SDF file would set? I'm not sure about the use case for this, could you write an example?

Just checking my understanding about default values. I'm not sure what the current behaviour is, but maybe something like this could work?

When value is missing	has default	no default
optional (required == 0)	Default is used	Field is omitted
required (required == 1)	Default is used, error is printed?	Fail hard?

This way, the SDF spec could define whether to fail softly with an error message or hard by not parsing, by defining a default value or not.

[azeey]

Having a required attribute with a default value can be useful when an SDFormat element or attribute changes from being optional to being required and we want to tick-tock the change by assigning a default value. So, what @chapulina suggested in the table sounds good to me.

I would support removing the default values we currently have, but I'm afraid it might break some things.