OpenAPI 3.1: visitors after the dereference visitor don't get mutated elements

[glowcloud]

Content & configuration

Swagger/OpenAPI definition:

openapi: 3.1.0
info:
  title: Example generation test
  version: 1.0.0
paths:
  /foo:
    get:
      responses:
        '200':
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Foo'
components:
  schemas:
    Foo:
      type: object
      properties:
        bar:
          allOf:
            - type: object
              properties:
                a:
                  type: integer

Describe the bug you're encountering

The allOf properties are not merged into the parent schema if the schema was referenced. The dereferenced schema in the response will still have the allOf keyword:

while the schema itself will be merged:

The issue here is that the parent elements get mutated by the DereferenceVisitor:

swagger-js/src/resolver/apidom/reference/dereference/strategies/openapi-3-1-swagger-client/visitors/dereference.js

Lines 776 to 780 in fa77bfd

	if (isMemberElement(parent)) {
	parent.value = referencedElement; // eslint-disable-line no-param-reassign
	} else if (Array.isArray(parent)) {
	parent[key] = referencedElement; // eslint-disable-line no-param-reassign
	}

and the subsequent visitors do not see this change.

We get the correct result if instead of merging the AllOfVisitor with the DereferenceVisitor and visiting here:

swagger-js/src/resolver/apidom/reference/dereference/strategies/openapi-3-1-swagger-client/index.js

Lines 107 to 118 in fa77bfd

	if (this.mode !== 'strict') {
	const allOfVisitor = new AllOfVisitor({ options });
	visitors.push(allOfVisitor);
	}
	// establish root visitor by visitor merging
	const rootVisitor = mergeAllVisitorsAsync(visitors, { nodeTypeGetter: getNodeType });
	const dereferencedElement = await visitAsync(refSet.rootRef.value, rootVisitor, {
	keyMap,
	nodeTypeGetter: getNodeType,
	});

we did a second merge and visit with only the AllOfVisitor on the dereferenced element:

  if (this.mode !== 'strict') {
    const allOfVisitor = AllOfVisitor({ options });
    secondVisitors.push(allOfVisitor);
   }

   // establish root visitor by visitor merging
   const rootVisitor = mergeAllVisitorsAsync(visitors, { nodeTypeGetter: getNodeType });

   let dereferencedElement = await visitAsync(refSet.rootRef.value, rootVisitor, {
    keyMap,
    nodeTypeGetter: getNodeType,
   });

   const secondRootVisitor = mergeAllVisitorsAsync(secondVisitors, {
    nodeTypeGetter: getNodeType,
   });

   dereferencedElement = await visitAsync(dereferencedElement, secondRootVisitor, {
    keyMap,
    nodeTypeGetter: getNodeType,
   });

because we get updated elements.

Expected behavior

The updated elements need to be passed to the visitors after dereferencing, as this issue will affect all of them.

[char0n]

The root issue is that we're mutating parent and subsequent visitors have no idea about this change. This is a regression introduced by integrating ApiDOM Dereference Architecture 2.0.

We have couple of options how to resolve this:

1. issue second traversal - but we can get into the same situation as before, because one of the plugin can make significant changes that other plugins will not see
2. merge all visitors into single big one and perform everything into single pass - there will be no separation of concerns anymore
3. introduce a signal mechanism that will detect significant tree modifications and will dispatch subsequent visitors in separate passes. We don't have this mechanism currently because we explicitly avoided it. It makes performance unpredictable. But I can imagine if you don't care about performance (CLI usage for example), you just turn it on with option and don't care how your visitors really look like.
4. using a nested sync traversal only for dereferenced fragments using visitor produced by merging all-of, parameters and properties visitors.

---

Decision: we will go for option 4. It requires the least amount of effort to fix the issue, it requires minimal repeated traversal of the same nodes and is aligned of how ApiDOM traversal was original design to be performed.

[char0n]

I've researched standardized approach to the problem we currently have - it's called requeueing the traversal and I'm currently in the process to implement it in ApiDOM. There are two approaches to the requeueing:

1. Automatic
2. Manual

Babel is using automatic requeueing as it has an abstraction facilitated via NodePath.

Our traversal mechanism is more generic and not bound to particular structure (it handles parser CSTs, ASTs and ApiDOM). It was originally build as immutable, but for dereferencing we needed to utilize mutations. Every time the mutation is introduced that significantly modifies the ApiDOM tree (e.g. replacing current node), we need to manually dispatch requeueing traversal.

---

For solving this issue we technically didn't need complex requeuing mechanism. ApiDOM traversal natively supported immutable current node replacement. In swagger-api/apidom#4120 we added support for mutable current node replacement - this is all that we currently need.

Upstream ApiDOM issue swagger-api/apidom#4120

[char0n]

Addressed in 2eb34c9

[char0n]

Released in https://github.com/swagger-api/swagger-js/releases/tag/v3.28.1