Fix/adfa 4514 [KDoc-to-JSON plugin]

[alexmmiller]

Plugin for Dokka that intercepts the HTML rendering process to output raw JSON data in its place.

Example output with sourceSet whitelisting: "kotlin-jvm-common-json.zip" attachment at https://appdevforall.atlassian.net/browse/ADFA-4737

[davidschachterADFA]

From Claude AI:

Issues found

1. omitNulls config option almost certainly doesn't work as documented.
   In JsonPluginConfig.kt:

kotlin
@SerialName("omit-nulls")
val omitNulls: Boolean = false,

Every other field in this data class relies on its default (property-name-matching) serial name — this is the only one with a custom @SerialName, and it maps to kebab-case "omit-nulls" instead of "omitNulls". But the README's own config example (section 3) and its config table both show:

json
{ ..., "omitNulls": true }

Since parsing goes through kotlinx.serialization either via Dokka's native config decoder or the manual Json { ignoreUnknownKeys = true } fallback in JsonRenderer.kt, a user who copies the README's example verbatim will have "omitNulls" silently treated as an unknown key, and omitNulls will stay at its default false — with no error, since ignoreUnknownKeys swallows it. This seems like a stale/accidental annotation (every other field name should probably work here) rather than an intentional key rename. Worth fixing the annotation (or the docs, if kebab-case really is intended) and probably adding a quick round-trip test on JsonPluginConfig to catch this class of bug in future.

2. README section 7 ("Resolving Cross-Module Links") describes behavior the code doesn't actually implement.
   The README says an unresolved DRI "means the LinkPostProcessor failed to find that DRI" and implies you'd inspect the output for unresolved:... strings to debug it. But in LinkPostProcessor.kt:

kotlin
val replaced = unresolvedRegex.replace(text) { matchResult ->
val dri = matchResult.groupValues[1]
val resolved = driIndex[dri]
if (resolved != null) { ...; "$rootPrefix$resolved" } else { "#" }
}

Every unresolved: gets rewritten in this same pass — resolved ones become a relative path, unresolved ones become a bare "#". So the final on-disk JSON never actually contains an unresolved: string to grep for; the documented troubleshooting technique (search rendered JSON for unresolved:) won't find anything even when links failed to resolve. If diagnosing failed resolutions is meant to be possible, the plugin needs to log which DRIs fell through to "#" (it currently doesn't — only a final aggregate "Successfully resolved $replacedCount..." count is logged, with no count/list of failures), or the docs should stop implying you can find them by inspecting output.

3. Minor: inconsistent error handling between LinkPostProcessor's two passes.
   Pass 1 (building the DRI index) wraps each file in try/catch and logs+continues on failure. Pass 2 (the actual rewrite) has no such guard — a single unreadable/unwritable file (permissions, disk full, concurrent modification) throws and aborts the whole post-process step, leaving every other file's links unpatched even though the index was already built successfully. Worth matching pass 1's resilience here, especially since this runs as the last step of a potentially long Dokka build.

4. Minor: driIndex[dri] = fullUrl in extractDris silently last-writer-wins if the same DRI resolves to a URL in more than one file (plausible with expect/actual declarations across source sets). Not necessarily wrong, but undocumented — a one-line comment on why last-wins is fine (or a warning when it happens) would help a future reader trust this is intentional rather than an oversight.

On the "sourceSet whitelisting" commits

The last two commits (sourceSet whitelisting, Whitelist sourceSets per ADFA-4737) look purposeful and match a real ticket, and the whitelist logic itself (in JsonRenderer/ModelMapper) looks correct from what I read. I'd suggest double-checking verify_sourceset_whitelist.py gets run against a real kotlin-stdlib build with a whitelist configured, as part of this PR's test plan — I didn't see that confirmation anywhere in the PR description (there's no "Test plan" checklist here at all, unlike #21).

Suggestion: given the size (10 commits, an entire new Gradle module) and no test plan in the description, it'd help reviewers if the PR description at least linked a sample run's output (e.g. from scripts/build-example.sh or test_kotlin_stdlib.sh) so they don't have to build the whole thing locally to sanity-check items 1–2 above.

[alexmmiller]

Addressed David's comment, added tests for config options, added a test plan I'm going to start working through

[alexmmiller]

Implemented all automation specified in TEST_PLAN.md. Actual testing against Dokka base kotiln-stdlib can be done manually.

[luisguzman-adfa]

Hello @alexmmiller please find here the review I can provide;

PR #18 — review comments

Ran build-example.sh + run_all.sh on ARM64/Ubuntu 24.04: build OK, 14/14 test scripts pass.

The following comes from this review, each is a Suggestion (concrete fix) or a Decision point (design call to make). Most useful first:

1. Accessor/param links resolve to dead # (design call).
The example logged Failed to resolve 28 DRIs (patched to "#") — getters/setters, ctor params. resolveUrl (ModelMapper 24-40) emits unresolved: for synthetic accessors, then LinkPostProcessor (45-51) turns anything not in the index into "#".

Decision point: either resolve those DRIs to their owner's page (strip the getter/setter/PointingToCallableParameters suffix, look up the parent DRI) so they become real links, or document them as intentional non-links. In a 10-type fixture this already produced 28 dead links, so at real-library scale it could leave many references non-navigable.

2. href is not escaped — injection risk. extractText (ModelMapper 178-183) builds <a href="$href"> from KDoc; a " in a link breaks/injects into the HTML that ships inside the JSON.

Suggestion: Escape the attribute value (" → ").

3. Hard cast can crash mapping. mapProjection(...) as VarianceDto (ModelMapper 224) throws ClassCastException if Dokka returns Star/a bare bound.

Suggestion: as? VarianceDto ?: InvarianceDto(...).

4. Render loop isn't resilient like LinkPostProcessor now is. Page writes (JsonRenderer 178-207) have no try/catch, so one unwritable file aborts the whole build's output.

Suggestion: Wrap per-file write in try/catch + warn, matching the hardening already done in LinkPostProcessor.

5. mapExtras reflects on Dokka internals by class/method name (DefaultValue, AdditionalModifiers, ObviousMember…), swallowing failures into debug. This silently degrades when someone bumps Dokka (which the README invites).

Suggestion: Mark it version-coupled with a comment + add one assertion against the pinned Dokka so a break is loud.

6. omitNulls also strips empty strings/arrays/objects, not just nulls (JsonRenderer 247-252) — so functions: [] disappears.

Decision point: Note this in the README (consumers must treat missing keys as empty), or split it as omitEmpty.

7. README §7 is stale. It says grep the JSON for unresolved:, but nothing on disk contains it anymore — the info is in the Failed to resolve N DRIs log.

Suggestion: Point the doc at the log warning.

8. Preview-API warning at JsonOutputPlugin.kt:18

Suggestion: add @OptIn(DokkaPluginApiPreview::class).

9. Gradle drift (infra). Plugin uses wrapper 9.1.0, example uses 9.5.1 (two downloads); both warn they break on Gradle 10 (Kotlin 1.9.x plugin).

Suggestion: Align the wrappers; open a follow-up to bump the Kotlin plugin before Gradle 10.

PR description: add the test-plan and confirmation that verify_sourceset_whitelist.py was run against a real kotlin-stdlib build are not yet there — worth adding, along with the sample run (14/14)