Repo Type Integrity Checkers at Check Time

[zmoskala]

Summary

• Repository type integrity checkers now run at check time. IntegrityCheckerFactory.getTextUnitCheckers resolves a set union of type-owned and repository-owned checker types whose assetExtension matches the asset path's extension, so the same (assetExtension, integrityCheckerType) configured in both places is instantiated once, while different checker types on the same extension all run. Untyped repositories keep using only their own -it checkers.
• The type-owned half is loaded by an explicit JPQL query on AssetIntegrityCheckerRepository keyed by repository id plus asset extension. The check path is reached with detached assets, so resolving through asset.getRepository().getRepoType() would depend on a lazy association being initialized; the query avoids that and returns an empty set for an untyped repository.
• The assignment is read on every check rather than copied into repository config, so assigning a type, clearing it, or editing a type's checkers changes the next import with no repository-level change. This covers every path that goes through the factory: XLIFF import, localized-asset import, batch import, workbench checks, and pseudolocalization.
• repo-view stays repository-only for -it config: type-owned checkers print on a separate Repository type checkers --> line and are never merged into Integrity checkers -->. They come from a follow-up RepoTypeClient.getRepoTypeById because the nested repoType on the repository payload is still only id + name. If that GET fails, repo-view prints Repository type checkers --> could not be loaded and still prints the rest of the repository (id, type name, -it checkers, locales). repo-create / repo-update help now states that -it stores checkers on the repository only and that an assigned type also runs its own.
• No new REST endpoint and no schema change — the type-owned rows already existed on the repo-type API. Docs updated: the integrity-checkers and creating-repository guides describe the union, and the runtime superset moves out of "out of scope" in Architecture and off the ROADMAP list. CLI and UI for managing a type's checkers is still follow-up.

Extra

Not part of the original union work. I ran into this while walking through local manual tests on this PR.

In Test E I created a repo that had properties:MESSAGE_FORMAT both on the type and on -it, imported a broken ICU string (that part was fine — rejected, English source still in the pull), then imported a valid translation. That second import died with An unexpected error happened and a unique-constraint violation on tm_text_unit_current_variant (UK__TM_TEXT_UNIT_ID__LOCALE_ID). Same crash on Test C’s follow-up (untyped repo, only -it), so this is not the type/repo union.

What was going on: a failed integrity check still writes a current-variant row (just not included in the localized file). The next file import is supposed to update that row. Instead it treated the locale as empty, because the “are there already translations?” check used the inheritance cache, and that cache ignores rejected strings. So it tried to insert a second current variant for the same text unit + locale.

The change is small: look at actual current-variant rows for that asset and locale, including rejected ones, before skipping the lookup. First import of a locale that truly has none still skips the per-string lookup. Automated coverage: AssetIntegrityCheckerServiceTest#testLocalizedAssetImportAfterRejectedIntegrityCheckUpdatesCurrentVariant (broken properties import, then a valid one; current variant becomes the good string and is included).

Test plan

Manual

Verified against a locally built webapp only — default in-memory HSQL, no extra Spring config, nothing deployed. All CLI calls go through the alias below, which pins the client at localhost:8080.

The thing to watch in almost every test is the pulled French file, not the CLI output. An import that fails an integrity check still says Finished: the translation is stored, just marked as not includable. So if a checker rejected the target, pull writes the English source instead, and if nothing rejected it, the broken French shows up in the file.

Setup (webapp, CLI alias, fixtures)

./mvnw -pl webapp,cli -am -DskipTests package

# terminal 1 — webapp on default in-memory HSQL
java -jar webapp/target/mojito-webapp-*-SNAPSHOT-exec.jar

# terminal 2 — CLI pinned at the local webapp
alias mojito-local='java \
  -Dl10n.resttemplate.host=localhost \
  -Dl10n.resttemplate.port=8080 \
  -Dl10n.resttemplate.scheme=http \
  -Dl10n.resttemplate.authentication.username=<username> \
  -Dl10n.resttemplate.authentication.password=<pwd> \
  -jar '"$PWD"'/cli/target/mojito-cli-*-SNAPSHOT-exec.jar'

Two separate source directories, because push uploads every matching file and import then expects a localized file for each asset:

mkdir -p tmp/integrity-checkers/{src-msg,src-ellipsis,fr,out}
cd tmp/integrity-checkers
printf '%s\n' 'greeting=Hello {name}'      > src-msg/messages.properties
printf '%s\n' 'greeting=Bonjour {name}'    > fr/messages_fr-FR.good.properties
printf '%s\n' 'greeting=Bonjour {name'     > fr/messages_fr-FR.broken.properties
printf '%s\n' 'ellipsis=Hello {name}…'     > src-ellipsis/ellipsis.properties
printf '%s\n' 'ellipsis=Bonjour {name}...' > fr/ellipsis_fr-FR.properties

MESSAGE_FORMAT rejects the missing } in {name. ELLIPSIS rejects ASCII ... when the source uses …. A target that is valid ICU but uses ... is what separates the two checkers.

There is no CLI flag for a type's checkers, so types are created with mojito-local api against /api/repo-types. The import/pull loop below is the same everywhere — copy a localized file into the source dir, import, delete it, pull, read the result:

mojito-local push   -r <repo> -s src-msg -ft PROPERTIES
cp fr/messages_fr-FR.broken.properties src-msg/messages_fr-FR.properties
mojito-local import -r <repo> -s src-msg -ft PROPERTIES
rm src-msg/messages_fr-FR.properties
mojito-local pull   -r <repo> -s src-msg -t out/<repo> -ft PROPERTIES
cat out/<repo>/messages_fr-FR.properties

Config and help

• A — help text. repo-create --help and repo-update --help both say -it stores checkers on this repository only, that an assigned type also runs its checkers, and that an empty --repo-type means untyped / clears the assignment.
• B — plain untyped repo, no checkers. Baseline: with nothing configured, nothing runs. Imported greeting=Bonjour {name and the pull contains that broken target verbatim.
• C — untyped repo with -it properties:MESSAGE_FORMAT. The old behavior still works. Same broken import, and the pull falls back to greeting=Hello {name}.

The actual new behavior

• D — repo has no -it, its type has properties:MESSAGE_FORMAT. This is the whole point of the change: the type's checker alone rejects the broken target, and repo-view shows it on Repository type checkers --> with no Integrity checkers --> line.
• E — same properties:MESSAGE_FORMAT on both the type and -it. Overlap is safe: broken target rejected, valid target accepted, no errors. A second run of the same checker would look identical in the pull, so this does not prove the factory built only one instance. (The valid-import half is what surfaced the bug described under Extra; it passes on the fixed build.)
• F — different checkers on each side, both directions. Type MESSAGE_FORMAT + repo ELLIPSIS, then type ELLIPSIS + repo MESSAGE_FORMAT. In both cases a target that is valid ICU but uses ... is rejected — so neither side quietly overrides the other.
• G — typed repo whose type has no checkers. Assigning a type must not switch off what the repository already had. Its own -it MESSAGE_FORMAT still rejected the broken target.
• H — type configured for a different extension. Type has xliff:MESSAGE_FORMAT, the asset is messages.properties, so nothing applies and the broken target ships. Checkers are matched against the asset's extension, not applied to everything.
• I — changing the assignment between imports. Five imports on one repository, each with a different broken target so it registers as a new translation: untyped → accepted; assign the type → rejected; clear the type → accepted; assign a type with no checkers → accepted; PATCH /api/repo-types/{id} to give that type a checker → rejected. No repository-level -it change anywhere in that sequence.

Every path that resolves checkers

• J — Workbench. On a type-only repository, saving Bonjour {name} works and saving Bonjour {name raises the integrity warning — so this isn't just a CLI-import code path.
• K — POST /api/textunitsBatch. A broken placeholder sent through the batch importer is stored but stays out of the pulled file.
• L — pseudolocalization. pseudo produced greeting=⟦萬萬萬 Ήēĺĺō {name} 國國國⟧ — the placeholder survives untouched.
• M — XLIFF tm-export / tm-import. Exported the TM, broke the closing brace on the French target, re-imported, and the broken string stayed out of the pull. Note tm-export writes both a source and a _fr-FR XLIFF; tm-import has to point at a directory holding only the localized one.

Not covered here: any non-local server, a CLI for editing a type's checkers (REST only in this PR), drop-based Project Request import, the role-based permissions on PATCH /api/repo-types, and counting checker invocations from a pulled file.

Automated

./mvnw -pl webapp -am -Dtest=AssetIntegrityCheckerServiceTest -Dsurefire.failIfNoSpecifiedTests=false spotless:check test

• AssetIntegrityCheckerServiceTest (ServiceTestBase, real DB): untyped repository with no checkers, type-only, repo-only, and overlapping the same pair (import still rejects — not a count of instances); disjoint pairs in both directions (type checker rejects what the repo checker would allow, and the reverse); a typed repository whose type has an empty checker set still runs its own checkers; assigning a type, clearing it, and editing a type's checkers between two imports of the same text unit; and the workbench check, localized-asset import, and pseudolocalization paths driven by a type-only checker. Also: after a rejected localized-asset import, a later valid import updates the current variant instead of 500’ing (testLocalizedAssetImportAfterRejectedIntegrityCheckUpdatesCurrentVariant).
• AssetIntegrityCheckerServiceTest#testFactoryUnionsCheckersUsingDetachedAssetAndFiltersByExtension is the “instantiated once” check: MESSAGE_FORMAT is configured on both the type and the repository, plus two other checkers, and the factory returns three instances (not four). Same test asserts extension filtering and that the lazy repoType on a detached asset is not initialized first.
• TextUnitBatchImporterServiceTest#testTypeOnlyIntegrityCheckerIsUsedOnBatchImport: a type-owned MESSAGE_FORMAT excludes a broken placeholder on batch import, so the union applies to asyncImportTextUnits and not just the XLIFF path.
• RepoViewCommandTest (CLITestBase, real HTTP): Repository type checkers appears only when the assigned type has checkers, is absent for an untyped repository and for a type with none, and a repository -it checker added afterwards prints on Integrity checkers without the two lines collapsing into one.
• RepoViewTypeCheckersLoadFailureCommandTest: if loading type checkers fails, repo-view prints could not be loaded and still prints id, type name, and locales.
• RepoCreateCommandTest#testCreateHelpDocumentsTypeIntegrityUnion: help documents both halves of the contract — -it stores repository checkers only, and an assigned type also runs its checkers — so dropping either sentence fails the test.

[ehoogerbeets]

My only comment is a nitpick, so approving this. You can update the code or not. Your choice.

[ehoogerbeets]

Nitpick

Il y a deux fichiers => There are two files

This is in the singular plural choice.

[zmoskala]

thanks! fixed