[Bug Fix] @Image directive requires an explicit file extension while images described in Markdown do not(SR-15315)

[Kyle-Ye]

Bug fix: SR-15315

https://bugs.swift.org/browse/SR-15315

Summary

Starting a Local Preview Server and set a breakpoint at ResourceReference's path property

Remove the ".png" to trigger the pause.

And find only 2 API get the path property.
One is inside DocumentationContext.swift

swift-docc/Sources/SwiftDocC/Infrastructure/DocumentationContext.swift

Lines 1997 to 2007 in ff972f1

	public func resource(with identifier: ResourceReference, trait: DataTraitCollection = .init()) throws -> Data {
	guard let bundle = bundle(identifier: identifier.bundleIdentifier),
	let assetManager = assetManagers[identifier.bundleIdentifier],
	let asset = assetManager.allData(named: identifier.path) else {
	throw ContextError.notFound(identifier.url)
	}
	let resource = asset.data(bestMatching: trait)
	return try dataProvider.contentsOfURL(resource.url, in: bundle)
	}

The other is inside RenderNodeTranslator.swift

swift-docc/Sources/SwiftDocC/Model/Rendering/RenderNodeTranslator.swift

Lines 1221 to 1277 in ff972f1

	/// Creates a render reference for the given media and registers the reference to include it in the `references` dictionary.
	mutating func createAndRegisterRenderReference(forMedia media: ResourceReference?, poster: ResourceReference? = nil, altText: String? = nil, assetContext: DataAsset.Context = .display) -> RenderReferenceIdentifier {
	var mediaReference = RenderReferenceIdentifier("")
	guard let media = media else { return mediaReference }
	let fileExtension = NSString(string: media.path).pathExtension
	func resolveAsset() -> DataAsset? {
	renderContext?.store.content(
	forAssetNamed: media.path, bundleIdentifier: identifier.bundleIdentifier)
	?? context.resolveAsset(named: media.path, in: identifier)
	}
	// Check if media is a supported image.
	if DocumentationContext.isFileExtension(fileExtension, supported: .image),
	let resolvedImages = resolveAsset()
	{
	mediaReference = RenderReferenceIdentifier(media.path)
	imageReferences[media.path] = ImageReference(
	identifier: mediaReference,
	// If no alt text has been provided and this image has been registered previously, use the registered alt text.
	altText: altText ?? imageReferences[media.path]?.altText,
	imageAsset: resolvedImages
	)
	}
	if DocumentationContext.isFileExtension(fileExtension, supported: .video),
	let resolvedVideos = resolveAsset()
	{
	mediaReference = RenderReferenceIdentifier(media.path)
	let poster = poster.map { createAndRegisterRenderReference(forMedia: $0) }
	videoReferences[media.path] = VideoReference(identifier: mediaReference, altText: altText, videoAsset: resolvedVideos, poster: poster)
	}
	if assetContext == DataAsset.Context.download, let resolvedDownload = resolveAsset() {
	// Create a download reference if possible.
	let downloadReference: DownloadReference
	do {
	mediaReference = RenderReferenceIdentifier(media.path)
	let downloadURL = resolvedDownload.variants.first!.value
	let downloadData = try context.dataProvider.contentsOfURL(downloadURL, in: bundle)
	downloadReference = DownloadReference(identifier: mediaReference,
	renderURL: downloadURL,
	sha512Checksum: Checksum.sha512(of: downloadData))
	} catch {
	// It seems this is the way to error out of here.
	return mediaReference
	}
	// Add the file to the download references.
	mediaReference = RenderReferenceIdentifier(media.path)
	downloadReferences[media.path] = downloadReference
	}
	return mediaReference
	}

The first will call DataAssetManager and will use inner fuzzyKeyIndex to query which have no problem.
The second is where the bug comes from. It need to get the extension of the media to check if is supported.

My 2 potential fix suggestion:

1. Create some helper method in "DataAssetManager" and "DocumentationContext" to use fuzzyKeyIndex to get the file extension Which seems ugly.
2. Assume a no-file-extension media must be a supported image and then skip the check. (Which I do in the commit)

Dependencies

NO

Testing

Steps:

1. Download sample project https://developer.apple.com/documentation/xcode/slothcreator_building_docc_documentation_in_xcode
2. Remove any @image'source string of ".png" in the tutorial file of the sample project.
3. Build documentation to verify fix

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

[franklinsch]

Thanks for these changes! It's looking great. I just have one small suggestion regarding the doc comment. Also, we’ll want to add tests to check the new behavior.

[Kyle-Ye]

Rebase to the updated main branch.
Update the doc comment and add the related test.
Any suggestion on the test function? @franklinsch

[franklinsch]

Thanks for adding the test! It tests the behavior of the API in DocumentationContext , but it doesn’t cover that RenderNodeTranslator actually calls it with the correct parameters, i.e., the test would still pass if we weren’t specifying the type argument to this function in RenderNodeTranslator. Does that make sense?

I’d recommend implementing another test in RenderNodeTranslatorTests to verify similar assertions of your existing test. Thanks so much for doing this work and iterating on it, I really appreciate it!

[Kyle-Ye]

Thanks for adding the test! It tests the behavior of the API in DocumentationContext , but it doesn’t cover that RenderNodeTranslator actually calls it with the correct parameters, i.e., the test would still pass if we weren’t specifying the type argument to this function in RenderNodeTranslator. Does that make sense?

I’d recommend implementing another test in RenderNodeTranslatorTests to verify similar assertions of your existing test. Thanks so much for doing this work and iterating on it, I really appreciate it!

Did not find a file named RenderNodeTranslatorTests.swift and add the test into SemaToRenderNodeTests.swift

[franklinsch]

I think we should be asserting on the RenderNode node here, similar to other tests, rather than the state of the translator. I don't think it's expected for us to have separate image references for introposter and introposter.png, since they refer to the same asset, right? I think we should use always use the full file name with extension as the key.

[Kyle-Ye]

The RenderNode seem to be always true, do you mean we assert the renderNode's references property or something else?

let renderNode = translator.visit(technology) as! RenderNode
guard let introPosterReference = renderNode.references["introposter.png"] as? ImageReference else {
    XCTFail("Missing intro poster reference")
    return
}

I don't think it's expected for us to have separate image references for introposter and introposter.png, since they refer to the same asset, right?

Yes, they refer to the same asset. But due a implementation detail, they have different key, will try to solve the wrong behavior in the coming commit.

[Kyle-Ye]

Fix the test issue, but for the second problem I think we need the resolveAsset(named name: String, in parent: ResolvedTopicReference, ofType type: AssetType? = nil) method to not only return a DataAsset? but also the queried "full media name"/"file extension" String from the inner bestKey method. Is that OK with you?

/// Finds the best matching storage key for a given asset name.
/// `name` is one of the following formats:
/// - "image" - asset name without extension
/// - "image.png" - asset name including extension
func bestKey(forAssetName name: String) -> String? {
    guard !storage.keys.contains(name) else { return name }
    
    // Try the fuzzy index
    return fuzzyKeyIndex[name]
}

[franklinsch]

Hmm interesting. If we create a new function in the context, it would need to find the correct DataAssetManager to query again. Maybe it would be reasonable to add a property in DataAsset (which resolveAsset returns) that tracks the identifier that should be used when referring to this asset. That way, we resolve the asset and get its identifier in one call.

Something like this in DataAsset:

/// The identifier of the asset.
///
/// This is typically the base name of the asset and its extension, for example "`image.png`".
public var identifier: String

We'd deprecate the existing DataAsset initializer and create a new one that takes an identifier, and update usages of the DataAsset initializer to use the new one that takes an identifier.

[Kyle-Ye]

Yes, it is a way to achieve it. But can we wrap and expose the bestKey method like the updated PR? So we'll get the fullname of the asset. Which do you prefer?

[franklinsch]

We could expose the bestKey functionality (which I think we would name differently in DocumentationContext) but I think starting to store the identifiers in DataAsset instead makes sense long term. That way, clients don't need to query the context twice (once to resolve, another to get the identifier). I think it's expected that you'd get a canonical identifier for it as part of the resolution process.

Interested in hearing your thoughts though!

[Kyle-Ye]

Yes, I think from long term we need such identifier property in DataAsset, I'm currently trying to implement it. But encounter some problems here.

Sure, we can use dataAsset.identifier here to indicate the name.

But in some case the DataAsset(identifier:) seem hard to use/fit the situation.

And sometimes it causes duplication.

Also after the trying, I change my mind and think DataAsset actually don't fit to add a identifier property. Since it is an abstract of "Asset" and should not contain the key.

[Kyle-Ye]

And we can view and discuss the WIP change on the branch here if needed

[franklinsch]

I think the changes you're showing the screenshot make sense—for the VideoReference decoder implementation, the identifier we should use is the identifier value declared a few lines above, since that's the value we're encoding. Regarding the duplication, I'm not too concerned about that, it turns out we're also using the identifier as the key of a dictionary for fast lookups.

[Kyle-Ye]

Update with the "nit" suggestion

[franklinsch]

@swift-ci please test

[franklinsch]

Looks great, thank you @Kyle-Ye !