Repository: common-workflow-language/schema_salad

Issue:
schema-salad-doc produces duplicated attribute entry when inheriting from parent
Ref: https://github.com/common-workflow-language/cwl-v1.2/pull/166#issuecomment-1164571015

The latest release of `schema-salad` allows for overriding the documentation of a parent type in a child type. i.e.

```yaml
- 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
```

Note the `id` field has a different `doc` value in the child type. However, running this schema through `schema-salad`, it prints the `id` field twice in the child.

```bash
(venv) kinow@ranma:~/Development/python/workspace/schema_salad$ schema-salad-tool --print-doc schema_salad/tests/inherited-attributes.yml > /tmp/test.html 
/home/kinow/Development/python/workspace/schema_salad/venv/bin/schema-salad-tool Current version: 8.3.20220622140130
```

![image](https://user-images.githubusercontent.com/304786/175427997-7ad943d5-8b60-4a6d-bc7c-a6d2f539a347.png)


Discussion:
kinow: The unit test that was failing initially now passes with the latest commit. And the generated HTML looks correct to me.

![image](https://user-images.githubusercontent.com/304786/175435224-e47f5860-998f-48db-8080-ef98cee36696.png)

kinow: Ah, will try to fix the tests broken by this change :+1: 
kinow: There were 6 unit tests failing. 5 were due to the output of the fields written to the traceback. That's because instead of getting all the parent fields, and then calling `exfields.extend(types)`, I had picked the subtype fields first, and then added the ones that were exclusive to the parent, which altered the order fields were written in tracebacks, and also in the codegen.

I reverted it, using two loops to keep the existing behaviour.

Now it's down to a single unit test, `test_avro_loading_subtype_bad`, that is still failing. It is failing, from what I could tell, because we handle part of the fields inheritance/extends in `schema_salad/schema` (called from `makedoc`), and another part in `avro/schema`.

The `avro/schema` is currently raising an exception if the type in extends has an incompatible type to the type it is extending (i.e. Child extends Parent, and in Child doc is an int, but in Parent doc is a string). The current code also relies on having both child types, and parent types. So my fix, which removes the duplicated types from parents, broke this behavious since `avro/schema` now has only the fields that are not repeated. I **think** it should be OK to move that logic to `schema_salad/schema`, and that should fix the test.

But I will put that in a separate commit to make it easier to revert and try something else if needed :+1: 
codecov[bot]: # [Codecov](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) Report
> Merging [#556](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) (e213be7) into [main](https://codecov.io/gh/common-workflow-language/schema_salad/commit/b65373eca1b71a532bcc3193134668d8feab6d96?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) (b65373e) will **increase** coverage by `0.14%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #556      +/-   ##
==========================================
+ Coverage   82.57%   82.71%   +0.14%     
==========================================
  Files          20       20              
  Lines        3920     3929       +9     
  Branches     1078     1058      -20     
==========================================
+ Hits         3237     3250      +13     
+ Misses        442      440       -2     
+ Partials      241      239       -2     
```


| [Impacted Files](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language) | Coverage Δ | |
|---|---|---|
| [schema\_salad/avro/schema.py](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-c2NoZW1hX3NhbGFkL2F2cm8vc2NoZW1hLnB5) | `66.86% <ø> (-0.39%)` | :arrow_down: |
| [schema\_salad/schema.py](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-c2NoZW1hX3NhbGFkL3NjaGVtYS5weQ==) | `77.64% <100.00%> (+0.88%)` | :arrow_up: |
| [schema\_salad/makedoc.py](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language#diff-c2NoZW1hX3NhbGFkL21ha2Vkb2MucHk=) | `84.67% <0.00%> (+1.11%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language). Last update [b65373e...e213be7](https://codecov.io/gh/common-workflow-language/schema_salad/pull/556?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=common-workflow-language).

tetron: I think previously, the CWL spec just avoided re-declaring fields like this, and narrowing the range of a type on a concrete record was accomplished using the specialization feature.

So what's new is that we allow re-declaring a field as long as it's type is a compatible subset of the parent, which is useful.

I haven't tested the code but it looks reasonable.  

My one note is that it appears to rely on the order-preserving property of Python dicts, which is de facto part of the language since 3.5 and officially part of the language from Python 3.7, but is a bit subtle, so we should make a note of that.
kinow: >LGTM, but it would be best if @tetron also reviewed this

Good idea. Thanks @mr-c 

>I think previously, the CWL spec just avoided re-declaring fields like this, and narrowing the range of a type on a concrete record was accomplished using the specialization feature.
>
>So what's new is that we allow re-declaring a field as long as it's type is a compatible subset of the parent, which is useful.
>
>I haven't tested the code but it looks reasonable.

:+1: 

>My one note is that it appears to rely on the order-preserving property of Python dicts, which is de facto part of the language since 3.5 and officially part of the language from Python 3.7, but is a bit subtle, so we should make a note of that.

Note added.

Thanks @tetron !
kinow: (rebased)

Patch:
diff --git a/schema_salad/avro/schema.py b/schema_salad/avro/schema.py
index e02e61f..8af6202 100644
--- a/schema_salad/avro/schema.py
+++ b/schema_salad/avro/schema.py
@@ -503,15 +503,6 @@ class RecordSchema(NamedSchema):
                 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}")
diff --git a/schema_salad/schema.py b/schema_salad/schema.py
index 8652445..2e8a253 100644
--- a/schema_salad/schema.py
+++ b/schema_salad/schema.py
@@ -34,7 +34,7 @@ from schema_salad.utils import (
 )
 
 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"]:


Test patch:
diff --git a/schema_salad/tests/inherited-attributes.yml b/schema_salad/tests/inherited-attributes.yml
new file mode 100644
index 0000000..d37a827
--- /dev/null
+++ b/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
diff --git a/schema_salad/tests/test_makedoc.py b/schema_salad/tests/test_makedoc.py
new file mode 100644
index 0000000..41fa8d6
--- /dev/null
+++ b/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")
diff --git a/schema_salad/tests/test_subtypes.py b/schema_salad/tests/test_subtypes.py
index 1e0c56d..6d9473a 100644
--- a/schema_salad/tests/test_subtypes.py
+++ b/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(