Index external entities after local entities

[anferbui]

Bug/issue #, if applicable: rdar://163018922

Summary

The navigation indexing code skips all further nodes after it has already indexed a node with a given identifier 1.

This can result in an odd interaction when there is an external link which is resolved to a path that the bundle serves itself (or more colloquially, a catalog has an external link to itself).

Since external entities are indexed before local entities, they were always be preferred over local entities, resulting in local entities mistakenly being dropped from the navigator altogether. The resulting navigator is incorrect and missing nodes, due to external entities not having hierarchy information. This issue was introduced when support for external links in the navigator was introduced.

This PR updates ConvertActionConverter to always index local entities first. If any external entities clash with local entities, they will be resolved as the local entity (including its hierarchy) in the navigator.

Dependencies

N/A

Testing

Use the test bundle from Example.zip.

Steps:

1. Run DOCC_LINK_RESOLVER_EXECUTABLE=Example/test-data-external-resolver swift run docc preview Example/Example.docc
2. Verify that http://localhost:8080/documentation/example-bundle/articlea has two links in the Topics section, and that the first one resolves to a local entity, not the external entity.
3. Verify the navigation hierarchy contains ArticleC, curated in local node ArticleB.

This can in general be verified by adding an external link to a page's content, which links to an entity that resolves to the same relative presentation URL of a local entity. If the resulting navigator is unchanged when adding this external links vs removing it, then the code is working as expected.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

• Added tests
• Ran the ./bin/test script and it succeeded
• [ ] Updated documentation if necessary N/A

[anferbui]

Open question: Do we need any further logging / assertions / checking logic when we skip a node when building the navigator?

swift-docc/Sources/SwiftDocC/Indexing/Navigator/NavigatorIndex.swift

Lines 711 to 713 in b27288d

	guard identifierToNode[normalizedIdentifier] == nil else {
	return nil // skip as item exists already.
	}

[anferbui]

@swift-ci please test

[d-ronnqvist]

This fix looks good to me but I think it'd be good to find a different location for this test and see if it can use TestExternalReferenceResolver instead of a real on-disk script.

[anferbui]

@d-ronnqvist I've moved the test to Tests/SwiftDocCTests/Indexing/ExternalRenderNodeTests.swift and changed it to not use a link resolver script. Also updated the comment phrasing to match your suggestions.

[anferbui]

@swift-ci please test

[d-ronnqvist]

This looks good to me.

I think the test can be simplified by reusing an existing test resolver kind but we can make that change in a follow up PR at some points.

[anferbui]

@swift-ci please test