comments on resolve-allof (#3657)

layouts/partials/json-schema/resolve-allof.html

@@ -12,11 +12,21 @@
   Of course the parent can itself inherit from *its* parent, so we recurse to
   handle that.
 
+  Note that `allOf` is only resolved at the top level of the schema object. For
+  example, if you call this on an API definition which defines a `parameter`
+  which has an allOf schema, it will not be resolved. To handle this, the
+  openapi templates call resolve-allof for every schema object that they
+  process.
 */}}
 
 {{ $ret := . }}
 {{ $original := . }}
 
+{{/*
+  We special-case 'required', and accumulate the values from all the 'allOf'
+  entries (rather than simply overriding them). Start the accumulation here.
+*/}}
+
 {{ $required := .required }}
 {{ if not $required }}
     {{ $required := slice }}
@@ -24,13 +34,12 @@
 
 {{ with $ret.allOf }}
 
-    {{ $all_of_values := dict }}
-
     {{/*
-      allOf is always an array
+      construct a new dict, with each of the allOf entries merged into it in
+      turn.
     */}}
+    {{ $all_of_values := dict }}
     {{ range . }}
-
         {{ with .required }}
             {{ $required = union $required . }}
         {{ end }}
@@ -38,33 +47,50 @@
         {{/*
           With merge, values from the second argument override those from the first argument.
           So this order will accumulate values from allOf items, allowing later ones to override earlier
+
+          Note also that `merge` does a *deep* merge - nested maps are also
+          merged. (Slices are replaced though.)
         */}}
         {{ $all_of_values = merge $all_of_values . }}
-
     {{ end }}
 
     {{/*
-      Then apply allOf values to the original, but allow the original to override allOf.
+      Finally, merge in the original, allowing the original to override allOf.
     */}}
     {{ $ret = merge $all_of_values $ret }}
 
     {{/*
-      Except that if allOf *itself* contains allOf, we do want to override the original for that field only.
+      Except that if allOf *itself* contains allOf (ie, the parent also
+      inherits from a grandparent), then we replace allOf in the original
+      with that in the parent. Below, we see that this has happened, and
+      recurse.
+
+      TODO: surely it would be better to simply do the recursion as we iterate
+      though the allOf list above - not least because we might have multiple
+      parents with different grandparents, and by doing this we only get one
+      set of grandparents.
     */}}
     {{ with $all_of_values.allOf }}
         {{ $ret = merge $ret (dict "allOf" . ) }}
     {{ end }}
 
+    {{/*
+       special-case 'required': replace it with the union of all the
+       'required' arrays from the original and allOf values.
+
+       XXX: but first we merge in the original 'required', again? why
+          do we do that? it should already have been done at the start.
+    */}}
     {{ with $ret.required }}
         {{ $required = union $required $ret.required }}
     {{ end }}
 
     {{ $ret = merge $ret (dict "required" $required) }}
-
 {{ end }}
 
 {{/*
-  Recurse while we are finding new allOf entries to resolve
+  If we replaced the 'allOf' dict with one from a grandparent, we now
+  need to recurse.
 */}}
 {{ if ne $ret.allOf $original.allOf }}