Change Tomcat selection to be driven by manifest pin.

[mshlyukarski]

Related to issue: #1155.

[ramonskie]

Code Review

Thanks for this PR — the root cause is correctly identified and the fix is well-scoped. We also confirm we already had JDK-based Tomcat selection in place (the >= 11 → 10.x / < 11 → 9.x logic), but the user's explicit JBP_CONFIG_TOMCAT version pin was never consulted. Below are our findings.

---

🔴 Critical: DetermineTomcatVersion extracts the wrong digit in many real-world configs

The regex (\d+) runs on the entire JBP_CONFIG_TOMCAT string and returns the first digit it finds — not necessarily the Tomcat version. This silently selects the wrong Tomcat version for common configs:

Example 1 — access logging only (no version pin):

JBP_CONFIG_TOMCAT="{access_logging_support: {access_logging: enabled}}"

→ regex matches "1" from "enabled" → returns "1.x" → selection overrides JDK-based logic with a nonsense version

Example 2 — external configuration enabled:

JBP_CONFIG_TOMCAT="{tomcat: {external_configuration_enabled: true}, ...}"

→ regex matches "1" from "true" → returns "1.x" → same problem

In both cases the function should return "" (no pin), but instead returns a garbage result that silently takes over the JDK-based selection path.

Suggested fix — parse specifically from the tomcat.version key, or better yet use YAML parsing (consistent with how the Ruby buildpack reads JBP_CONFIG_TOMCAT):

// Example: targeted regex instead of greedy first-digit match
re := regexp.MustCompile(`tomcat\s*:\s*\{[^}]*version\s*:\s*["']?(\d+)`)
match := re.FindStringSubmatch(configEnv)

---

🔴 Critical: The error return value of DetermineTomcatVersion is never checked

tomcatVersion, err := common.DetermineTomcatVersion(os.Getenv("JBP_CONFIG_TOMCAT"))
// err is never inspected before using tomcatVersion
if tomcatVersion == "" { ...

The function signals failure via err, but the call site only checks tomcatVersion == "". The error return is misleading — callers will assume a non-empty tomcatVersion means a valid result was parsed, which is not true (see Critical #1 above). Either check the error and log a warning, or change the signature to return only a string (empty = no pin found).

---

🟡 Warning: DetermineTomcatVersion belongs in tomcat.go, not common/context.go

The function is Tomcat-specific parsing of JBP_CONFIG_TOMCAT. The common package is for shared buildpack infrastructure (Context, Stager, Manifest interfaces, VCAP helpers). Placing a component-specific config parser there pollutes the shared package.

All the other JBP_CONFIG_TOMCAT parsers (isAccessLoggingEnabled, extractRepositoryRoot, extractVersion) already live in tomcat.go — this function should too.

---

🟡 Warning: No compatibility guard when the user pins an incompatible combination

The Tomcat compatibility matrix states Tomcat 10 requires Java 11+. If a user explicitly pins version: "10.+" with Java 8, the new code silently allows it — Tomcat will fail to start at runtime, not at staging. A warning during staging would help:

if tomcatMajor == 10 && javaMajorVersion < 11 {
    t.context.Log.Warning("Tomcat 10.x requires Java 11+, but Java %d detected. Tomcat may fail to start.", javaMajorVersion)
}

---

🔵 Suggestion: No unit tests for DetermineTomcatVersion

Given the function has non-trivial parsing behaviour, unit tests are essential. There is currently no context_test.go. Key cases to cover:

Input	Expected output
""	""
"{tomcat: { version: \"9.+\" }}"	"9.x"
"{access_logging_support: {access_logging: enabled}}"	"" ← currently broken
"{tomcat: {external_configuration_enabled: true}}"	"" ← currently broken
"{tomcat: { version: \"10.+\" }}"	"10.x"

---

✅ What the PR does well

• Correctly identifies and targets the root cause
• Minimal change — doesn't refactor unrelated code
• The fallback chain is preserved: explicit pin → JDK-based selection → DefaultVersion
• Integration tests are meaningful and test the right scenario
• Log message now consistently shows both Tomcat version and Java version

---

The fix is on the right track, but the DetermineTomcatVersion regex needs to be made precise before merge to avoid silently breaking users who set other JBP_CONFIG_TOMCAT options without a version pin.

[mshlyukarski]

Hi, thanks for the review comments.
They are fixed based on the following arguments:

• DetermineTomcatVersion extracts the wrong digit in many real-world configs
  Suggested
  re := regexp.MustCompile(tomcat\s*:\s*\{[^}]*version\s*:\s*["']?(\d+))
  match := re.FindStringSubmatch(configEnv)

Used:
re := regexp.MustCompile((?i)tomcat\s*:\s*\{[\s\S]*?version\s*:\s*["']?([\d.]+\.\+))
match := re.FindStringSubmatch(raw)

The Regex now runs on the tomcat string and gets the version pattern of the tomcat, which later is filtered by the libbuildpack.
Since, it is tied to how libbuildpack handles the version pattern after that. This way it is corrected implemented, same as the logic of the Ruby buildpack.
Covered by the unit tests.

• The error return value of DetermineTomcatVersion is never checked
  Its error return value is not used at all right now. In practice, it always returns nil, and Supply() ignores the err result entirely.
  If JBP_CONFIG_TOMCAT: '{access_logging_support: {access_logging: enabled}}}' is placed, the returned result is logically correct “”.
  So it is better to remove the error from determineTomcatVersion (and its exported wrapper) and update the callers and tests accordingly. That makes the API clearer, the call sites simpler, and reflects the actual behavior of the function.
  The error below is covered by the libbuildpack library.

DetermineTomcatVersion belongs in tomcat.go, not common/context.go
Correctly moved inside tomcat.go
Created a wrapper, in order to be used for the unit tests.

• No unit tests for DetermineTomcatVersion
  Unit tests created for the key cases to cover.

• No compatibility guard when the user pins an incompatible combination
  if tomcatMajor == 10 && javaMajorVersion < 11 {
  t.context.Log.Warning("Tomcat 10.x requires Java 11+, but Java %d detected. Tomcat may fail to start.", javaMajorVersion)
  }

• Added integration test for the particular scenario (the incompatible one)

• Added strict compatibility:

  • Tomcat 10.x requires Java 11+.
  • Tomcat 9.x supports Java 8–22.
    A clear Warning pops up if someone forces 10.x on Java < 11 via JBP_CONFIG_TOMCAT.
    During staging:
    WARNING Tomcat 10.x requires Java 11+, but Java 8 detected. Tomcat may fail to start.
    Most probably the staging will fail.