slachiewicz commented on issue #736: URL: https://github.com/apache/maven-doxia-sitetools/issues/736#issuecomment-6770471170
### Independent review of the proposed design An independent pass over this issue's proposal, checked against the current sources of `maven-doxia-sitetools` (2.x), `maven-site-plugin` and `doxia`. File/line references are to those sources as of today. **Overall: the design holds up.** The proposed shape (read-only view + opt-in interface, called by `DefaultSiteRenderer` after `doxia.getParser(...)` and before the `ParserConfigurator`) is the right one. The review found three items that should be corrected in the issue text before implementation, plus confirmations of the squeezier claims. Ordered by importance: #### R1 — `getOutputFile()` cannot be implemented as drafted `renderDocument(...)` never sees the output directory: only `Renderer.render(documents, siteContext, outputDirectory)` receives it, and it is not stored in `SiteRenderingContext` (`DefaultSiteRenderer.render()` computes `new File(outputDirectory, docRenderer.getOutputName())` locally, then throws it away). So the proposed `Path getOutputFile()` has no source of truth. Recommendation: expose the **site-root-relative** output path instead — `DocumentRenderingContext.getOutputPath()` already computes it (`index.md` → `index.html`, Velocity extensions stripped) — and treat DOXIASITETOOLS-357 as solved by that. Diagram-type Asciidoctor extensions write images relative to the *source* document, so `getSourceFile()` already covers them. If an absolute path is ever needed, add an optional `outputDirectory` field to `SiteRenderingContext` and let `render(...)` populate it; don't put it in the interface until then. #### R2 — `getProperties(): Map<String, String>` misrepresents what template attributes actually contain `AbstractSiteRenderingMojo.createSiteRenderingContext()` (around lines 276–297) builds the template attributes as `Map<String, Object>` containing: - the **`MavenProject` itself** under the reserved key `"project"` (for the Velocity templates — `SiteMojo` templates render `$project.…`), - `"inputEncoding"` / `"outputEncoding"` strings, - all of `project.getProperties()` (raw `Properties`, `Object` values), - the user's `attributes` site-plugin parameter as overrides, applied last. If the proposed view exposes template attributes as `Map<String, String>` verbatim, it silently (a) lies about the value type and (b) re-hands the `MavenProject` to parsers through the side door this issue exists to close. Parsers that just read project properties don't need the object. Recommendation: define the view's properties viewpoint precisely — e.g. a copy of the string-valued entries with the reserved keys (`project`, `inputEncoding`, `outputEncoding`) excluded, overridable by the user's `attributes` — and document the resolution order matching what the Velocity context does today. Alternatively type it `Map<String, Object>` and exclude `project` explicitly. Either way, spell out the reserved keys in the Javadoc. #### R3 — "parsers are looked up per document, so per-document state is safe" needs rewording As stated it is not accurate. `DefaultParserManager.getParser(id)` returns components from a `Map<String, Parser>` injected **once** (`DefaultParserManager.java:35–49`): every document, and every thread, gets the **same parser instance**. The same is true for existing per-document mutation in `DefaultSiteRenderer.renderDocument()` (`setEmitComments`/`setEmitAnchorsForIndexableEntries`, lines 463–467). What makes mutable parser state work today is that rendering of documents within a site rendering is sequential — `site:run`'s `DoxiaHandler.render()` explicitly renders one document at a time on the render thread and waits (`CompletableFuture.get()`). This doesn't invalidate the design — the proposed setter is no more racy than what the renderer already does — but the contract must be written explicitly, something like: > The context is set on the parser immediately before the document is parsed. `renderDocument(...)` must not invoke the same parser instance concurrently. Parser implementations may keep a reference to the context for the duration of that document's parse, not as global state. #### Confirmations (the less obvious claims check out) - `SiteRenderingContext` really is immutable in the aggregate API but mutable in *contents*: `getAttributes()` returns an unmodifiable view (`Map<String, Object>`, `@since 2.1.0`), with `putAttribute`/`removeAttribute` for callers — but the rest is all setters (`setTemplateProperties`, `setSiteModel`, `setParserConfigurator`, …). Handing the raw object to parsers would expose shared, cross-document mutable state; the **read-only view is the better option** of the two the issue offers. Prefer a view interface over passing `DocumentRenderingContext` + `SiteRenderingContext` directly: `DocumentRenderingContext` also has public `setAttribute()` (used by the renderer to flag Velocity documents), so even it is not read-only. - `<custom>` of the site descriptor is a strong candidate for the Maven-independent configuration home: typed `Xpp3Dom` in `doxia-site-model`, already merged across parent/child by `DefaultSiteModelInheritanceAssembler`, already read in production by skins vía `$site.getCustomValue(...)` (see the Fluido conditional-resources precedent). Pair it with a documented transitional key in the attributes map for the site-goal configuration DOM, deposited by whichever mojo actually runs (which also removes the Asciidoctor parser's implicit hard-coding of the `site` goal key, which would not resolve under `site:run` today). The plain `Map<String, String>` plugin parameter (option 2 of the issue) should be dropped: lossy against Asciidoctor's option shape, new user plumbing, and buys nothing `<custom>` doesn't already give. - Views since `2.1.0` free-form attributes map: confirmed (`@since 2.1.0` on `getAttributes`/`putAttribute`/`removeAttribute`). #### Suggested refinements to the interface - Add `String getParserId()` and `String getExtension()` (cheap, and spares parsers re-deriving format facts); not strictly needed day one. - `getProperties()` type and semantics per R2. - Keep implementation package-private, one view per document, constructed in `renderDocument(...)` — additive, no doxia-core change needed (single-component release), consistent with `ParserConfigurator`. #### Plan review The three-step plan (Sitetools interfaces + test parser → Site Plugin sets attributes + bump → asciidoctor-maven-plugin implements the interface with injection fallback) is the right order and remains compatible with MSITE-849/#1350: once Asciidoctor stops reading `MavenProject`, the render-executor workaround in `DoxiaHandler` can be revisited. --- *Review notes: default of the two `Xpp3Dom` passing conventions (attributes key vs `<custom>`) is proposer's call to settle in the issue; R2's reserved-key list (`project`, `inputEncoding`, `outputEncoding`) should be validated against any other consumer that reads template attributes directly.* -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
