Prevent duplicated fields from parents in child types (#556)

Author: kinow

schema_salad/avro/schema.py

@@ -503,15 +503,6 @@ def make_field_objects(field_data: List[PropsType], names: Names) -> List[Field]
                 new_field = Field(
                     atype, name, has_default, default, order, names, doc, other_props
                 )
-                # make sure field name has not been used yet
-                if new_field.name in parsed_fields:
-                    old_field = parsed_fields[new_field.name]
-                    if not is_subtype(old_field["type"], field["type"]):
-                        raise SchemaParseException(
-                            f"Field name {new_field.name} already in use with "
-                            "incompatible type. "
-                            f"{field['type']} vs {old_field['type']}."
-                        )
                 parsed_fields[new_field.name] = field
             else:
                 raise SchemaParseException(f"Not a valid field: {field}")

schema_salad/schema.py

@@ -34,7 +34,7 @@
 )
 
 from . import _logger, jsonld_context, ref_resolver, validate
-from .avro.schema import Names, SchemaParseException, make_avsc_object
+from .avro.schema import Names, SchemaParseException, make_avsc_object, is_subtype
 from .exceptions import (
     ClassValidationException,
     SchemaSaladException,
@@ -617,7 +617,7 @@ def extend_and_specialize(
                 for spec in aslist(stype["specialize"]):
                     specs[spec["specializeFrom"]] = spec["specializeTo"]
 
-            exfields = []  # type: List[str]
+            exfields = []  # type: List[Any]
             exsym = []  # type: List[str]
             for ex in aslist(stype["extends"]):
                 if ex not in types:
@@ -645,8 +645,46 @@ def extend_and_specialize(
 
             if stype["type"] == "record":
                 stype = copy.copy(stype)
-                exfields.extend(stype.get("fields", []))
-                stype["fields"] = exfields
+                combined_fields = []
+                fields = stype.get("fields", [])
+                # We use short names here so that if a type inherits a field
+                # (e.g. Child#id) from a parent (Parent#id) we avoid adding
+                # the same field twice (previously we had just
+                # ``exfields.extends(stype.fields)``).
+                sns_fields = {shortname(field["name"]): field for field in fields}
+                sns_exfields = {
+                    shortname(exfield["name"]): exfield for exfield in exfields
+                }
+
+                # N.B.: This could be simpler. We could have a single loop
+                #       to create the list of fields. The reason for this more
+                #       convoluted solution is to make sure we keep the order
+                #       of ``exfields`` first, and then the type fields. That's
+                #       because we have unit tests that rely on the order that
+                #       fields are written. Codegen output changes as well.
+                #       We are relying on the insertion order preserving
+                #       property of python dicts (i.e. relyig on Py3.5+).
+
+                # First pass adding the exfields.
+                for sn_exfield, exfield in sns_exfields.items():
+                    field = sns_fields.get(sn_exfield, None)
+                    if field is None:
+                        field = exfield
+                    else:
+                        # make sure field name has not been used yet
+                        if not is_subtype(exfield["type"], field["type"]):
+                            raise SchemaParseException(
+                                f"Field name {field['name']} already in use with "
+                                "incompatible type. "
+                                f"{field['type']} vs {exfield['type']}."
+                            )
+                    combined_fields.append(field)
+                # Second pass, now add the ones that are specific to the subtype.
+                for field in sns_fields.values():
+                    if field not in combined_fields:
+                        combined_fields.append(field)
+
+                stype["fields"] = combined_fields
 
                 fieldnames = set()  # type: Set[str]
                 for field in stype["fields"]:

schema_salad/tests/inherited-attributes.yml

@@ -0,0 +1,25 @@
+- name: Parent
+  doc: |
+    Parent record
+  documentRoot: true
+  docChild:
+    - "#Child"
+  type: record
+  fields:
+    - name: id
+      jsonldPredicate:
+        _id: "#id"
+      type: int
+      doc: Parent ID
+
+- name: Child
+  doc: |
+    Child record
+  type: record
+  extends: Parent
+  fields:
+    - name: id
+      jsonldPredicate:
+        _id: "#id"
+      type: int
+      doc: Child ID

schema_salad/tests/test_makedoc.py

@@ -0,0 +1,18 @@
+"""Test schema-salad makedoc"""
+
+from io import StringIO
+
+from schema_salad.makedoc import makedoc
+
+from .util import get_data
+
+
+def test_schema_salad_inherit_docs() -> None:
+    """Test schema-salad-doc when types inherit and override values from parent types."""
+    schema_path = get_data("tests/inherited-attributes.yml")
+    assert schema_path
+    stdout = StringIO()
+    makedoc(stdout, schema_path)
+
+    # The parent ID documentation (i.e. Parent ID) must appear exactly once.
+    assert 1 == stdout.getvalue().count("Parent ID")

schema_salad/tests/test_subtypes.py

@@ -101,9 +101,8 @@ def test_avro_loading_subtype_bad() -> None:
     path = get_data("tests/test_schema/avro_subtype_bad.yml")
     assert path
     target_error = (
-        r"Union\s+item\s+must\s+be\s+a\s+valid\s+Avro\s+schema:\s+"
-        r"Field\s+name\s+override_me\s+already\s+in\s+use\s+with\s+incompatible\s+"
-        r"type\.\s+org\.w3id\.cwl\.salad\.Any\s+vs\s+\['string',\s+'int'\]\."
+        r"Field name .*\/override_me already in use with incompatible type. "
+        r"Any vs \['string', 'int'\]\."
     )
     with pytest.raises(SchemaParseException, match=target_error):
         document_loader, avsc_names, schema_metadata, metaschema_loader = load_schema(