Skip to content

TEDEFO-5226 Build a model of what template lines display and render it once the template is parsed - #181

Merged
rousso merged 5 commits into
developfrom
feature/TEDEFO-4794-template-model
Oct 4, 2026
Merged

rousso merged 5 commits into
developfrom
feature/TEDEFO-4794-template-model

Conversation

@rousso

@rousso rousso commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

A model of what template lines display

TEDEFO-5226

The template translators now follow the pattern of the EFX rules translator: the walk of the parse tree builds a model of the view template, and the output is produced from that model once parsing is complete. Until now, each part of a template line was rendered into markup as soon as it was parsed, and the pieces were joined on the call stack.

The model, in eu.europa.ted.efx.model.templates:

  • ViewTemplate: the global declarations, the declared templates, and the body, summary and navigation sections (ViewTemplateSection).
  • ContentBlock: a template line with its nested lines. It now holds content templates instead of markup.
  • ContentTemplate: what a block displays, one for each WHEN alternative and one for the rest. DisplayContentTemplate is made of fragments, InvokeContentTemplate calls a declared template.
  • ContentTemplateFragment and its subclasses: text, value, label (from a key or from an expression), line break and hyperlink. Each renders itself through the MarkupGenerator.

Both template translators (SDK 1 and SDK 2) build this model. The MarkupGenerator interface does not change: Conditional is now only what composeFragmentDefinition receives, and Conditionals is removed.

Other changes

  • Declared templates are kept in the view template, as in the grammar, instead of among the lines of the body. The generated names of body blocks therefore no longer count the declarations: a first line after one declaration is now body01 instead of body02. The tests that pinned these names were updated.
  • Each translation now has its own TranslatorContext. The SDK 2 translator used the shared TranslatorContext.DEFAULT and changed its current section, which affected other translations.
  • TEDEFO-5227, in a separate commit: the text of a hyperlink is now escaped like other free text. A linked word containing < produced an XSL stylesheet that was not valid XML.

This pull request already includes develop with #180: the label fragment of a list of asset ids gets the quantity of TEDEFO-5208.

Validation

  • Before the old code was removed, both versions produced the same output for every template in the toolkit's tests, and the same XSL in the Notice Viewer for all view templates of SDK 2.0 (52), 1.0 (46) and 1.10 (46), apart from the numbering of label variables.
  • mvn test: all 1472 tests pass.
  • mvn javadoc:javadoc javadoc:test-javadoc: no errors.
  • Notice Viewer: all 88 tests pass with this version of the toolkit.

@rousso rousso self-assigned this Oct 1, 2026
@rousso
rousso requested a review from rouschr October 1, 2026 19:47

@rouschr rouschr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Parse-tree-to-model refactor for EFX template translators

The template translators (SDK 1 and SDK 2) previously rendered markup incrementally as the ANTLR parse tree was walked — each fragment was immediately converted to a Markup string and joined on the call stack. This PR introduces an intermediate model layer (eu.europa.ted.efx.model.templates) so the walk builds a structured representation of the template, and output is produced in one pass from that model once parsing is complete. This mirrors the pattern already used by the rules translator.

The model is a small hierarchy: ViewTemplate holds global declarations and sections; each section has a tree of ContentBlocks; each block holds a list of ContentTemplates (one per WHEN alternative, one for the otherwise case); each DisplayContentTemplate is made of typed ContentTemplateFragments that know how to render themselves. The MarkupGenerator interface is unchanged.

Watch for: the V1 translator does not reset section per translation in enterTemplateFile — calling renderTemplate twice on the same V1 instance would accumulate blocks across translations (likely). The PR description notes that merging order with PR #180 matters for the label-with-asset-list quantity field (confirmed, per PR description).

Verdict: could be APPROVED, but pending the items below, see 1 and 3.


Issues (4)

1. V1 accumulates state across calls

EfxTemplateTranslatorV1.section is a final class-level field, never reset. Calling renderTemplate twice on the same instance appends the second template's blocks to the first's, producing incorrect output on the second call. enterTemplateFile should reinitialize section (as V2 does with viewTemplate = new ViewTemplate()), or the class should be documented as single-use.

2. Merge-order dependency with PR #180

The PR description explicitly warns that whichever of #180 and #181 is merged second must pass the quantity to LabelFromExpressionContentTemplateFragment for the asset-list label case. Verify before merging.

3. assert used as production invariant guard

exitTemplateFile (V2) and enterTemplateFile (V1 and V2) use assert to enforce stack emptiness:

assert this.stack.empty() : "Stack should be empty at this point.";
final String output = this.viewTemplate.render(this.markup, this.translatorContext).script.trim();

assert this.blockStack.isEmpty() : UNEXPECTED_INDENTATION;
this.viewTemplate = new ViewTemplate();

assert is a no-op unless the JVM is launched with -ea. These guards are disabled in production — a wrong stack state silently produces corrupt output instead of failing fast. Replace with explicit IllegalStateException throws if these are real invariants the translator depends on.

4. Null-safety gaps in the new model classes

Several constructor parameters are passed through without any null check, and none of the new model classes carry @Nonnull/@Nullable annotations as required by the project style.

4a. TextContentTemplateFragment(String text) — no null guard.
If text is null, markupGenerator.escapeSpecialCharacters(null) is called at render time; behaviour depends on the markup generator implementation. In the one call site added by this PR:

new TextContentTemplateFragment(ctx.textBlock().getText())

ANTLR's getText() does not return null for a matched token, so this is low risk in practice — but the constructor itself offers no contract.

4b. HyperlinkContentTemplateFragment(ContentTemplateFragment fragment, StringExpression url) — no null guard on fragment.
fragment.render(...) is called unconditionally in render(). A null fragment would NPE at render time, not at construction time, with no indication of where the null came from.

4c. DisplayContentTemplate.prepend(ContentTemplateFragment fragment) — no null guard.
ArrayDeque.addFirst(null) throws a NullPointerException immediately (it does not permit nulls), so this fails fast — but with an opaque stack trace. An explicit precondition check would identify the caller.

4d. No @Nonnull/@Nullable annotations on any of the ten new model classes.
The project style (javax) requires these on fields, constructor parameters, and method signatures. None of the new classes carry them.

Note: Markup.script is null-safe — the Markup constructor already does this.script = script == null ? "" : script, so the .script.trim() chain in exitTemplateFile is safe.


High-level view

The model-first pattern removes the tight coupling between parse-tree traversal and markup generation. Fragment subclasses each own the rendering call they previously triggered inline.

The WHEN/OTHERWISE ordering works through a deliberate reversal: ANTLR fires exit listeners in parse order, pushing each ContentTemplate onto the stack. exitTemplateLine drains the stack with addFirst, restoring parse order. Templates with a condition (set by exitWhenDisplayTemplate/exitWhenInvokeTemplate) become Conditionals; the otherwise template, which never gets setCondition called, becomes the default content. The contract is implicit — ContentTemplate's mutable setCondition is the only signal distinguishing a WHEN branch from the OTHERWISE branch.

The V2 translator's shared-singleton bug (TranslatorContext.DEFAULT was mutated across translations) is fixed by creating a new TranslatorContext() in enterTemplateFile. V1 creates its context locally in exitTemplateFile, but V1 still initializes section as a final class-level field — calling renderTemplate twice on the same V1 instance would accumulate blocks from both calls before rendering. The PR makes V2 safer but does not address V1.

Block numbering changes (body02 → body01 for the first body line following a declared template) are a correct side effect: declared templates are now stored in viewTemplate.getTemplateDeclarations() rather than the body section root. TEDEFO-5227 (hyperlink text escaping) rides in the same PR via TextContentTemplateFragment, confirmed by a new test.


Details

WHEN/OTHERWISE ordering and the mutable condition setter

ContentTemplate carries a mutable condition field (defaulting to BooleanExpression.empty()), set by exitWhenDisplayTemplate / exitWhenInvokeTemplate. The otherwise template is never passed to those handlers. ContentBlock.renderDefinition distinguishes them by contentTemplate.getCondition().isEmpty().

The ordering in exitTemplateLine:

while (stack.peek() instanceof ContentTemplate) {
    contentTemplates.addFirst(stack.pop(...));
}

ANTLR fires exit listeners in parse order; addFirst reverses them. After the loop, contentTemplates is in parse order: WHEN1, WHEN2, …, OTHERWISE. renderDefinition puts non-empty conditions into conditionals and the empty one into content.

V1 translator state reset

V2 resets both viewTemplate = new ViewTemplate() and translatorContext = new TranslatorContext() in enterTemplateFile. V1 does neither: section is a final ViewTemplateSection initialized at field declaration, and getTranslatedMarkup() drains the call stack but the section retains its children across calls. If V1 is always instantiated per translation (the typical injection pattern), this doesn't bite — but the asymmetry with V2 is worth addressing.

assert vs. hard failures

Both assert statements gate operations that would corrupt state if the invariant is violated — the render in exitTemplateFile would produce wrong output if the stack is not empty; the viewTemplate reset in enterTemplateFile would get a stale model if the block stack is not clear. Neither fires in a JVM without -ea. The existing IllegalArgumentException/IllegalStateException pattern used elsewhere in the translators is the right replacement here.

Null safety in the model layer

The new model classes are data holders with thin logic — their only responsibility is to hold their inputs and delegate to the markup generator. That makes null-at-construction the right place to fail: if the caller passes null, the problem is at the call site, not at render time two method calls later. Constructors for TextContentTemplateFragment, HyperlinkContentTemplateFragment, and InvokeContentTemplate should at minimum carry @Nonnull annotations; the ones that are genuinely nullable (none apparent here) should be @Nullable. DisplayContentTemplate.prepend should assert non-null explicitly.

ctx.textBlock().getText() in exitLinkedTextBlock is not a new call — it was already there before the refactor — but it is now asymmetric with the ctx.labelBlock() != null guard pattern used in the same method family, which is worth aligning for consistency.

Test coverage

Block-name updates (body02 → body01) account for most of the test diff, mechanical and correct. The new testLinkedTextBlock_WithSpecialCharacters_IsEscaped test confirms the TEDEFO-5227 fix.

Not covered: multi-call reuse of a single V1 translator instance; the assert-disabled path where the stack is non-empty at render time.


File map
File Change
Conditional.java Javadoc added; trivial accessor fix
Conditionals.java Deleted — replaced by List<ContentTemplate> at call sites
ContentBlock.java Conditionals + Markup fields replaced by List<ContentTemplate>; renderDefinition now builds conditionals/content from the list at render time
ContentBlockStack.java Signature update to match ContentBlock
ContentTemplate.java New — abstract base with mutable condition
ContentTemplateFragment.java New — abstract base for fragment types
DisplayContentTemplate.java New — fragment-list template; renders by iterating fragments in order
ExpressionContentTemplateFragment.java New — wraps an expression value
HyperlinkContentTemplateFragment.java New — wraps a fragment + URL
InvokeContentTemplate.java New — template invocation content
LabelContentTemplateFragment.java New — abstract label base with quantity
LabelFromExpressionContentTemplateFragment.java New — label from expression keys
LabelFromKeyContentTemplateFragment.java New — label from a single string key
LineBreakContentTemplateFragment.java New — line break
TemplateDefinition.java Signature update
TemplateInvocation.java Copies contentTemplates from the definition
TextContentTemplateFragment.java New — free text with escape
ViewTemplate.java New — top-level model: globals, declared templates, three sections
ViewTemplateSection.java New — one section's root block plus render logic
EfxTemplateTranslatorV1.java Builds model instead of markup; uses ViewTemplateSection directly
EfxTemplateTranslatorV2.java Full model-first rewrite; removes getTranslatedMarkup() and renderSection(); fixes shared TranslatorContext
EfxTemplateTranslatorV2Test.java Block name updates (body02→body01) and new hyperlink escaping test

@rouschr rouschr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

see point 1 and 3, other stuff can be of interest

@rousso

rousso commented Oct 3, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for the thorough review, Christophe.

  1. V1 keeping its state across calls: well spotted. This predated the refactor (on develop it applies to rootBlock), and it went further than the template lines: both translators also kept the indentation of the previous template, and left the blocks of its last nested lines on the block stack. A translator now starts each template from a clean state, and the end of the file closes every level still open. Tests cover reusing a translator in both versions.
  2. Merge order with TEDEFO-5208 Pass the quantity to pluralised label references with a sequence of asset ids #180: agreed. Whichever of the two is merged second will keep the fix from TEDEFO-5208 Pass the quantity to pluralised label references with a sequence of asset ids #180: the quantity is popped before checking whether the asset identifier is a sequence, and is then passed to LabelFromExpressionContentTemplateFragment.
  3. Assertions: these are intentional. They guard against errors in the translator itself, not against invalid templates, which is why they are assertions rather than exceptions. The one in enterTemplateFile is in fact what caught the leftover blocks mentioned in point 1.
  4. Null safety: the toolkit does not use @Nonnull or @Nullable anywhere, and has no dependency for them, so this is not part of the project's style. All the values concerned come from the parser, so we will leave the constructors as they are.

@rousso
rousso requested a review from rouschr October 3, 2026 02:07
@rousso
rousso merged commit 681970f into develop Oct 4, 2026
2 checks passed
@rousso
rousso deleted the feature/TEDEFO-4794-template-model branch October 4, 2026 11:57
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