Repository navigation
TEDEFO-5226 Build a model of what template lines display and render it once the template is parsed - #181
Conversation
rouschr
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
see point 1 and 3, other stuff can be of interest
|
Thank you for the thorough review, Christophe.
|
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.DisplayContentTemplateis made of fragments,InvokeContentTemplatecalls a declared template.ContentTemplateFragmentand its subclasses: text, value, label (from a key or from an expression), line break and hyperlink. Each renders itself through theMarkupGenerator.Both template translators (SDK 1 and SDK 2) build this model. The
MarkupGeneratorinterface does not change:Conditionalis now only whatcomposeFragmentDefinitionreceives, andConditionalsis removed.Other changes
body01instead ofbody02. The tests that pinned these names were updated.TranslatorContext. The SDK 2 translator used the sharedTranslatorContext.DEFAULTand changed its current section, which affected other translations.<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
mvn test: all 1472 tests pass.mvn javadoc:javadoc javadoc:test-javadoc: no errors.