fix(content): handle language-only locales without a country code#36405
fix(content): handle language-only locales without a country code#36405jcastro-dotcms wants to merge 10 commits into
Conversation
|
Claude finished @jcastro-dotcms's task in 2m 53s —— View job I'll analyze this and get back to you. |
🤖 dotBot Review (Bedrock)Reviewed 7 file(s); 3 candidate(s) → 2 confirmed, 0 uncertain (unverified, kept for review). Confirmed findings
us.deepseek.r1-v1:0 · Run: #28680764298 · tokens: in: 32834 · out: 7630 · total: 40464 · calls: 10 · est. ~$0.086 |
…Language Guava's ImmutableMap.Builder.put() throws NullPointerException on a null value. Language-only locales (no country code) legitimately have a null country/countryCode, which crashed any transform path that maps a contentlet's Language (new Edit Content save, History tab, GraphQL). Adds LanguageViewStrategyTest covering the null-country regression, the wrapAsMap=true path, the transform() entry point, and a non-regression check for locales that do have a country. Fixes #36106
Not related to the #36106 language locale fix in this PR. This addresses a separate, unrelated startup-time exception: VelocityUtil.getEngine() triggers a lazy Velocity engine init that touches the DB/company context, which on a fresh install can fire before Task00001LoadSchema has run, throwing a "No Company!" exception during boot. Deferring the probe until InitServlet sets dotcms.started.up=true avoids that noisy exception.
1054373 to
0156e31
Compare
VisitorAPIImpl.createVisitor() built a Locale via new Locale(language, country), which throws NullPointerException when country is null. Language-only locales (no country code) hit this on every page render, since VisitorAPIImpl.getVisitor() runs on every request. Language.asLocale() already null-guards this exact case (falls back to the 1-arg Locale constructor), so use it instead of constructing the Locale manually. Same underlying null-country data shape as the LanguageViewStrategy fix in this PR, different code path (page rendering / visitor creation vs. content transform).
PublishQueueElementTransformer.getMapForLanguage() built a java.util.Map.of(...) directly from language.getCountryCode(), which is null for a language-only locale (e.g. "es", no country). Map.of() throws NullPointerException on any null value, same failure mode as Guava's ImmutableMap.Builder fixed elsewhere in this PR. This crashed the Push Publishing queue view for any queued Language asset in a language-only locale. Defaults countryCode to blank before building the map, and adds PublishQueueElementTransformerTest covering the null-country regression and a non-regression check for locales with a country.
Addresses dotBot review feedback on PR #36405: the wrapAsMap=true and transform() test cases checked country/countryCode but not isoCode, leaving a gap in coverage for the ISO code formatting when country is absent.
|
Regarding the feedback from the dotBot Review (Bedrock): I went through all four findings and verified them against the actual code before acting. The two "Medium" isoCode findings are legitimate — good catch. The "Critical" finding is incorrect. It claims The "Medium" So: 2/4 findings addressed, 2/4 not applicable after verification. Happy to discuss further if I'm missing context on either of the latter two. |
VelocityHealthCheck.performCheck() now defers until dotcms.started.up=true before probing the engine. The test never set that property, so every test short-circuited on the deferred branch before reaching the mocked engine: returnsUpWhenRenderMarksResolves failed outright (expected UP, got DOWN), and the other two passed for the wrong reason, no longer exercising the literal-marker/engine-exception logic they're meant to cover. Sets dotcms.started.up=true in setUp() (restored in tearDown()) so the probe logic actually runs, and adds returnsDownWhenStartupNotComplete to cover the deferred branch itself, which had no test before.
|
@claude the issue in the This one doesn't hold up either. final String iso = UtilMethods.isSet(language.getCountryCode())
? language.getLanguageCode() + StringPool.DASH + language.getCountryCode()
: language.getLanguageCode();
builder.put("isoCode", iso.toLowerCase());So for Looks like the case-sensitivity concern is valid in principle for ISO 639-1/3166 formatting, but this codebase's convention (at least in this method) is to normalize the whole isoCode to lowercase, so the test is correct as written. |
|
Claude finished @jcastro-dotcms's task in 27s —— View job Verifying the
|
Summary
Core fix for #36106 — language-only locales (e.g.
es, no country code) crash multiple code paths because several places pass aLanguage's nullablecountry/countryCodestraight into null-hostile collection builders or theLocaleconstructor:LanguageViewStrategy.mapLanguage()built a GuavaImmutableMap.Builder, which throwsNullPointerException("null value in entry: country=null")on a null value — crashing any transform that maps a contentlet's Language (new Edit Content save, History tab, GraphQLLanguageDataFetcher). Fixed by defaultingcountry/countryCodeto""viaCollectionsUtils.orElseGet(), matching the existing pattern inLanguage.toMap().VisitorAPIImpl.createVisitor()built aLocalevianew Locale(language, country), which throwsNullPointerExceptionwhencountryis null — so a page could never render at all in a language-only locale (this runs on every request, via visitor creation). Fixed by switching toLanguage.asLocale(), which already null-guards this exact case and is already used elsewhere (LanguageWebAPIImpl,JsLanguage).PublishQueueElementTransformer.getMapForLanguage()built ajava.util.Map.of(...)directly fromlanguage.getCountryCode()—Map.of()rejects null values the same way Guava's builder does, crashing the Push Publishing queue view for any queuedLanguageasset in a language-only locale. Fixed the same way (default to blank before building the map).All three are the same underlying null-country data shape, hit through three independent code paths (response serialization, page rendering, Push Publishing queue).
Test coverage added:
LanguageViewStrategyTest— null-country regression (wrapAsMap=false/true), thetransform()entry point,isoCodeassertions on both, and a non-regression check for locales that do have a country.PublishQueueElementTransformerTest— null-country regression and non-regression check.Also included, unrelated to the #36106 fix above: a change to
VelocityHealthCheckthat defers its readiness probe untilWebKeys.DOTCMS_STARTED_UP("dotcms.started.up") is"true".VelocityUtil.getEngine()triggers a lazy Velocity engine init that touches the DB/company context, which on a fresh install can fire beforeTask00001LoadSchemaruns, throwing a noisy"No Company!"exception during startup. This just quiets that one-time startup exception.This guard broke
VelocityHealthCheckTest's existing assumptions: none of its 3 tests setdotcms.started.up, so every test was short-circuiting on the new deferred branch before ever reaching the mocked engine — one failed outright (returnsUpWhenRenderMarksResolves, expectedUPgotDOWN), and the other two were passing for the wrong reason, no longer exercising the logic they claim to test. Fixed by having the test set/restore that property around each run, and addedreturnsDownWhenStartupNotCompleteto cover the deferred branch itself, which had no coverage before.Why
System.getProperty()and notConfig.getStringProperty()for this flag:Configonly resolves values from three sources, in order — environment variables (DOT_*prefix), dotCMS's DB-backed "system table", anddotmarketing-config.properties/dotcms-config-cluster.propertiesfiles (seeConfig.java's own class javadoc andgetStringArrayProperty()). There is no code path inConfigthat ever reads a plain JVM system property set viaSystem.setProperty(...).dotcms.started.upis set exactly that way, inInitServlet.java, as a pure runtime sentinel flipped once at startup completion — it's never written to a config file, env var, or the system table. Routing the read throughConfig.getStringProperty(WebKeys.DOTCMS_STARTED_UP, ...)would mean this check could never see"true", so the readiness probe would report "startup not yet complete" forever, even long after dotCMS finished starting — a permanent regression, not a style improvement.System.getProperty(...)is also the existing, established pattern for reading this exact flag elsewhere (ServletContainerHealthCheck,MonitorHelper). The one change made here is using theWebKeys.DOTCMS_STARTED_UPconstant instead of the hardcoded"dotcms.started.up"string, in both the production code and its test.Closes #36106
Test plan
LanguageViewStrategyTest(4 tests) — reproduces the exact reportedNullPointerException: null value in entry: country=nullon the unpatched code, passes with the fixPublishQueueElementTransformerTest(2 tests) — reproduces theMap.of()NullPointerExceptionon the unpatched code, passes with the fixVelocityHealthCheckTest(now 4 tests) — verified it fails/mis-passes on the unpatched test, passes correctly after the fix./mvnw -pl :dotcms-core compile -DskipTests— compiles cleanVisitorAPIImplNPE (java.lang.NullPointerExceptionatLocale.<init>) by rendering a page in a language-only locale; confirmed fixed withLanguage.asLocale()es) and confirm no "Unknown Error" / 500Languageasset no longer 500s🤖 Generated with Claude Code