Skip to content

Fix false positive curation for article links with extra path components #1235

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

d-ronnqvist
Copy link
Contributor

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

Summary

This fixes a false positive curation for article links with extra path components.

Because the DocumentationCurator would previously only look at the last path component for articles, a link like <doc:nonsense/nonsense/nonsense/RealArticleName> would result in an association in the topic graph between doc://catalog.identifier/documentation/CatalogName/RealArticleName and the page that contains the curating topic section, even though DocC would fail to resolve the link (meaning that the page won't render that link in that topic section.

Dependencies

None.

Testing

Add extra nonsense path components to an article link in any topic section on any page (for example like in this test). That unresolved link shouldn't do anything.

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

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

case 0,1:
return NodeURLGenerator.Path.article(bundleName: bundle.displayName, articleName: path).stringValue
case 2:
return NodeURLGenerator.Path.documentationFolder + path
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return NodeURLGenerator.Path.documentationFolder + path
return NodeURLGenerator.Path.documentationFolder + "/" + path

Do we need to add a separator here to construct the article's URL properly? Running the new unit test I'm seeing this value for sourceArticlePath below: /documentationWrongModuleName/First. Looks like this would never match, even if the path was correct?

Or maybe I'm missing something? I would expect other unit tests to fail if we really needed the slash here, assuming we test article curation extensively already.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right that there should be a slash here. However, I couldn't find any test that reaches this code where the link isn't also expected to fail to resolve. I also didn't figure out how to write a new test that reaches this code when the link isn't expected to fail to resolve.

The pattern that we're trying to identify here is CatalogName/ArticleName but that link would have resolved earlier in this method, so it reach this code. Only when the catalog/module name is wrong do we reach this code but that link is expected to fail to resolve regardless. Ideally we'd be able to remove this entire fallback code path but that requires larger changes more suitable for a different PR.

@d-ronnqvist
Copy link
Contributor Author

@swift-ci please test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants