From e0d40c1d37c66c7c200ad4f49c69d6d24f9c10a5 Mon Sep 17 00:00:00 2001 From: bharat941 Date: Thu, 25 Jun 2026 08:32:51 +0530 Subject: [PATCH 1/4] feat(aem-cloud-service): add guava-cache analyzer detector + expert skill Adds the guava-cache code-assessment pattern (Guava cache -> Caffeine on AEM CS): - GuavaCache detector: import-anchored on com.google.common.cache.* (Java-only), registered in Registry.all(); exact prefix avoids the micrometer GuavaCacheMetrics false positive. - guava-cache/ expert skill: SKILL.md (control plane) + recipe.md (C1 pom swap, C2 imports, C3 builder/API mapping, test generation). - Catalog row in patterns.md (low | ready | analyzer | guided) + Manual Pattern Hints routing row in code-assessment SKILL.md. - Fixtures (legacy guava / clean caffeine / micrometer guard) + run-tests.sh block. Analyzer suite: 109 PASS / 0 FAIL (incl. [wiring] + guava block). --- .../skills/code-assessment/SKILL.md | 1 + .../code-assessment/guava-cache/SKILL.md | 63 ++++++ .../code-assessment/guava-cache/recipe.md | 193 ++++++++++++++++++ .../code-assessment/references/patterns.md | 1 + .../scripts/analyzer/Registry.java | 4 +- .../analyzer/detectors/GuavaCache.java | 41 ++++ .../guava-cache/CleanCaffeineCache.java | 23 +++ .../guava-cache/LegacyGuavaCache.java | 29 +++ .../guava-cache/MicrometerGuavaMetrics.java | 15 ++ .../test/code-assessment/run-tests.sh | 6 + 10 files changed, 375 insertions(+), 1 deletion(-) create mode 100644 plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md create mode 100644 plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md create mode 100644 plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java create mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java create mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java create mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java diff --git a/plugins/aem/cloud-service/skills/code-assessment/SKILL.md b/plugins/aem/cloud-service/skills/code-assessment/SKILL.md index 020dbb6f5..4a72e39be 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/SKILL.md +++ b/plugins/aem/cloud-service/skills/code-assessment/SKILL.md @@ -66,6 +66,7 @@ Route the request to one expert skill. Two pattern families share this skill: | `com.day.cq.replication.Replicator`, `org.apache.sling.replication.*`, "publish/preview activation" | [`replication/`](replication/SKILL.md) | `replication` | | `javax.jcr.observation.EventListener`, `org.osgi.service.event.EventHandler` on non-resource topics (replication, workflow, custom) | [`event-migration/`](event-migration/SKILL.md) | `eventListener` / `eventHandler` | | `com.day.cq.dam.api.AssetManager` create/upload/delete APIs, `createAssetForBinary`, `removeAssetForBinary` | [`asset-manager/`](asset-manager/SKILL.md) | `assetApi` | +| `import com.google.common.cache.*` (`CacheBuilder`, `LoadingCache`, `CacheLoader`), "guava cache", "swap guava for caffeine" | [`guava-cache/`](guava-cache/SKILL.md) | `guavaCache` | | HTL build warning `data-sly-test: redundant constant value comparison` | [`references/data-sly-test-redundant-constant.md`](references/data-sly-test-redundant-constant.md) | `htlLint` (reference, no expert skill subdirectory) | **Broad / correctness-review asks** ("check my Sling Models are implemented correctly", "review my code", "is my AEM project healthy", "assess this project") are not a single pattern: run the runbook in `discover` mode with intent `report` — the analyzer runs every detector and the report covers all built patterns, explicitly noting aspects not yet supported. Only narrow to one pattern when the user targets a specific fix. diff --git a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md b/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md new file mode 100644 index 000000000..4f2138cff --- /dev/null +++ b/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md @@ -0,0 +1,63 @@ +--- +name: guava-cache +description: AEM Cloud Service expert skill for the Guava cache → Caffeine swap. Covers bundles importing com.google.common.cache.* (Cache, CacheBuilder, LoadingCache, CacheLoader, RemovalListener). Classification, the near-1:1 Caffeine API mapping, pom dependency swap, import + call-site edits (getUnchecked→get, Callable→Function, RemovalNotification→3-arg), review checklist, and test generation. Routes detail to recipe.md. +license: Apache-2.0 +--- + +# Guava cache → Caffeine — AEM as a Cloud Service + +> This pattern is executed by the code-assessment runbook — follow [`../references/runbook.md`](../references/runbook.md) for the full flow (preflight → plan → apply → verify, run log). This skill supplies the detection + recipe the runbook applies. + +## Overview + +On AEM as a Cloud Service the supported in-process cache library is **Caffeine** (`com.github.benmanes.caffeine.cache.*`). Bundles importing `com.google.common.cache.*` are flagged because Guava is shrinking in the CS uber-jar and relying on Guava's cache from a third-party classloader is unstable. Caffeine is the recommended successor (same author as Guava cache) and its API is intentionally near-identical, so the swap is mechanical with a few well-known call-site renames. + +## Classification — confirm this pattern applies + +A file is in scope when it imports `com.google.common.cache.*` — `Cache`, `CacheBuilder`, `LoadingCache`, `CacheLoader`, `RemovalListener`, or `RemovalNotification`. + +1. **Bundle uses Guava cache** (import + real usage) → apply the full recipe: **C1 (pom)** + **C2 (imports)** + **C3 (builder / API call sites)**. +2. **Leftover import, no real usage** → just remove the dead `com.google.common.cache.*` import; no Caffeine dependency needed. +3. **Cache plus unrelated Guava utilities** (`com.google.common.collect.*`, `com.google.common.base.*`) → only the cache portion is in scope; leave other Guava usages alone unless the user asks. Keep the `guava` dependency if other code still imports `com.google.common.*`. + +**Not in scope (no false positive):** look-alike cache classes from other packages — e.g. `io.micrometer.core.instrument.binder.cache.GuavaCacheMetrics` — are not `com.google.common.cache.*` and are not flagged. + +## Discovery + +Detection is performed by the analyzer ([`../scripts/analyze.sh`](../scripts/README.md)), run by the runbook: + +```bash +bash ../scripts/analyze.sh --pattern guava-cache +``` + +**Match criteria (what the detector flags):** each `import com.google.common.cache.…` (explicit type or the package wildcard `.*`) in a parsed Java file, emitted with the import line and snippet. The match is an exact package-prefix on the import — Java-only, parse-level, import-anchored; no classpath or type resolution. + +## Resolution contract + +**self-evident** — the Guava → Caffeine API mapping is fixed (see [recipe.md](recipe.md)); no user input is required to plan the edit. The only judgment is the Caffeine version: pin it to the AEM CS SDK BOM (default `3.1.8`). + +## Review checklist + +- [ ] No `import com.google.common.cache.*` remains in changed files. +- [ ] No `CacheBuilder.newBuilder()` remains; all builders use `Caffeine.newBuilder()`. +- [ ] No `LoadingCache.getUnchecked(...)` remains; replaced with `.get(...)`. +- [ ] No `cache.get(key, Callable)` remains; the `Callable` is a `Function` (lambda). +- [ ] `RemovalListener` callbacks use the `(key, value, cause)` signature, not `RemovalNotification`. +- [ ] `Caffeine` is on the bundle's `pom.xml` with `provided`, version pinned to the AEM CS SDK BOM. +- [ ] `mvn clean install` passes; **aemanalyser** reports no `com.google.common.cache` API leaks. +- [ ] Guava dependency kept only if non-cache `com.google.common.*` usage remains. + +## Common pitfalls + +- **Embedding Caffeine** — use `provided`; Caffeine is supplied by the CS runtime, never embed it in the bundle. +- **Removing Guava too eagerly** — if other code still imports `com.google.common.collect/base`, keep the dependency and add Caffeine alongside. +- **`getUnchecked` left in place** — Caffeine has no `getUnchecked`; `LoadingCache.get(key)` already throws unchecked. +- **`Callable` vs `Function`** — `cache.get(key, …)` takes a `Function` in Caffeine, not a `Callable`. + +## Recipe + +Read [recipe.md](recipe.md) in full before editing: input contract, the C1/C2/C3 edits, the API mapping table, unlocatable / skip reasons, before/after examples, and test generation. + +## Handoff + +The skill never commits. See [`../references/git-workflow.md`](../references/git-workflow.md) for git vs in-place handoff and the suggested commit message. diff --git a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md b/plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md new file mode 100644 index 000000000..81874ec42 --- /dev/null +++ b/plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md @@ -0,0 +1,193 @@ +# Recipe — Guava cache → Caffeine + +> Read this fully before editing. Control plane: [SKILL.md](SKILL.md). + +## Input contract + +Per finding, regardless of how it was obtained: + +| Field | Example | Source | +|---|---|---| +| `file` | `core/.../UserCache.java` | Repo-relative path to the flagged Java file | +| `line` | `7` | Line of the `com.google.common.cache.*` import | +| `snippet` | `import com.google.common.cache.CacheBuilder;` | The flagged import | + +The fix parameters are **self-evident** — the Guava → Caffeine mapping below is fixed. The only choice is the Caffeine version: pin to the AEM CS SDK BOM (default `3.1.8`). + +## API mapping (Guava → Caffeine) + +| Guava | Caffeine | +|---|---| +| `com.google.common.cache.Cache` | `com.github.benmanes.caffeine.cache.Cache` | +| `com.google.common.cache.LoadingCache` | `com.github.benmanes.caffeine.cache.LoadingCache` | +| `com.google.common.cache.CacheBuilder` | `com.github.benmanes.caffeine.cache.Caffeine` | +| `com.google.common.cache.CacheLoader` | `com.github.benmanes.caffeine.cache.CacheLoader` | +| `com.google.common.cache.RemovalListener` | `com.github.benmanes.caffeine.cache.RemovalListener` | +| `com.google.common.cache.RemovalNotification` | callback signature `(K key, V value, RemovalCause cause)` | +| `CacheBuilder.newBuilder()` | `Caffeine.newBuilder()` | +| `.maximumSize(n)` / `.weakKeys()` / `.softValues()` / `.recordStats()` / `.removalListener(l)` | identical | +| `.expireAfterWrite(d, TimeUnit)` | `.expireAfterWrite(Duration)` (preferred) or `(d, TimeUnit)` | +| `.expireAfterAccess(d, TimeUnit)` / `.refreshAfterWrite(d, TimeUnit)` | `.expireAfterAccess(Duration)` / `.refreshAfterWrite(Duration)` | +| `.build()` | identical (returns `Cache`) | +| `.build(cacheLoader)` | identical (returns `LoadingCache`) | +| `cache.getIfPresent(key)` / `.asMap()` / `.invalidate(key)` / `.invalidateAll()` | identical | +| `cache.get(key, Callable)` | `cache.get(key, Function)` — argument is a `Function`, not `Callable` | +| `LoadingCache.getUnchecked(key)` | `LoadingCache.get(key)` — Caffeine's `get` already throws unchecked | +| `LoadingCache.refresh(key)` | identical | + +## C1 — Maven dependency swap (bundle pom) + +```xml + + + com.google.guava + guava + 31.1-jre + provided + +``` + +```xml + + + com.github.ben-manes.caffeine + caffeine + 3.1.8 + provided + +``` + +Rules: +- `provided` — Caffeine is supplied by the AEM CS runtime; never embed it. +- If Guava is also used elsewhere in the bundle (not just cache), **keep** the `guava` dependency and add Caffeine alongside. +- Pin the Caffeine version to whatever the AEM CS SDK BOM exports. + +## C2 — Imports + +```java +// REMOVE +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; +import com.google.common.cache.RemovalListener; +import com.google.common.cache.RemovalNotification; +``` + +```java +// ADD (only those actually used) +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.CacheLoader; +import com.github.benmanes.caffeine.cache.LoadingCache; +import com.github.benmanes.caffeine.cache.RemovalListener; +import com.github.benmanes.caffeine.cache.RemovalCause; +``` + +## C3 — Builder + API call sites + +```java +// BEFORE — Guava +LoadingCache users = CacheBuilder.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(10, TimeUnit.MINUTES) + .recordStats() + .build(new CacheLoader() { + @Override + public User load(String id) throws Exception { + return userRepo.findById(id); + } + }); + +User u = users.getUnchecked("u-42"); +``` + +```java +// AFTER — Caffeine +LoadingCache users = Caffeine.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(Duration.ofMinutes(10)) + .recordStats() + .build(id -> userRepo.findById(id)); + +User u = users.get("u-42"); +``` + +```java +// BEFORE — get-or-compute (Callable) // AFTER — Caffeine (Function) +String v = cache.get(key, () -> compute(key)); String v = cache.get(key, k -> compute(k)); +``` + +```java +// BEFORE — RemovalListener (RemovalNotification) +RemovalListener rl = notification -> + log.info("evicted {} cause={}", notification.getKey(), notification.getCause()); + +// AFTER — RemovalListener (3-arg) +RemovalListener rl = (key, value, cause) -> + log.info("evicted {} cause={}", key, cause); +``` + +## Unlocatable / skip + +- `import-not-found: com.google.common.cache.* not present in ` — the flagged import is no longer there (already migrated). Record `skipped`. +- `guava-still-required: non-cache com.google.common.* usage in ` — only remove the cache imports; keep the Guava dependency. Record as a partial apply note, not a skip. + +## Editing strategy + +Surgical, formatting-preserving text edits — no reformatting / re-serialization: +1. Replace each `com.google.common.cache.X` import with its Caffeine counterpart (C2); drop imports for types no longer referenced. +2. Replace `CacheBuilder.newBuilder()` → `Caffeine.newBuilder()` (C3). +3. Replace `getUnchecked(` → `get(`, and convert any `cache.get(key, Callable)` to a `Function` lambda. +4. Convert anonymous `CacheLoader` to a lambda where the `load` body is a single expression. +5. Swap the C1 pom dependency. + +Anchor each replace on the smallest unique substring so unrelated identical text is not touched. + +## Test generation + +After the swap, generate a JUnit test that confirms cache behaviour is preserved — `getIfPresent` returns `null` for unknown keys, `cache.get(key, Function)` computes and caches, `invalidate(key)` removes the entry, and `LoadingCache.get(key)` does not throw checked exceptions. One test class per production class changed, suffix `Test`, under `src/test/java/…`. + +```java +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; + +import org.junit.Before; +import org.junit.Test; + +public class UserCacheTest { + + private UserCache service; + + @Before + public void setUp() { + service = new UserCache(); + service.activate(java.util.Collections.emptyMap()); + } + + @Test + public void shouldReturnNullForUnknownKey() { + assertNull(service.getIfPresent("u-unknown")); + } + + @Test + public void shouldComputeAndCache() { + Object first = service.get("u-42"); + assertNotNull(first); + assertEquals(first, service.get("u-42")); + } + + @Test + public void shouldInvalidateEntry() { + service.get("u-42"); + service.invalidate("u-42"); + assertNull(service.getIfPresent("u-42")); + } +} +``` + +## See also + +- [`../references/aem-cloud-service-pattern-prerequisites.md`](../references/aem-cloud-service-pattern-prerequisites.md) — SCR → DS, service-user resolvers, SLF4J. +- Caffeine wiki: — behaviour differences (async loading, weight-based eviction) beyond this near-1:1 swap. diff --git a/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md b/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md index 2ca9a413d..bbac1e957 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md +++ b/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md @@ -39,6 +39,7 @@ Adding a pattern: add a row here, then build the expert skill from | [`asset-manager`](../asset-manager/SKILL.md) | migrate DAM `AssetManager` create/upload via Direct Binary Access (`@adobe/aem-upload`) and delete via in-JVM `resolver.delete()` + `commit()` or HTTP Assets API; removes `createAssetForBinary` / `getAssetForBinary` / `removeAssetForBinary` (not available on CS) | high | ready | analyzer | guided | | [`outbound-call-timeouts`](../outbound-call-timeouts/SKILL.md) | add connect/read/socket timeouts to outbound HTTP client construction (Apache HttpClient, OkHttp, JDK HttpClient) | high | ready | analyzer | mechanical | | [`unbounded-query`](../unbounded-query/SKILL.md) | bound or escalate an explicitly-unbounded query (`p.limit=-1` predicate / `setLimit(-1)`) — safe-cap where provable, else flag for pagination | high | ready | analyzer | guided | +| [`guava-cache`](../guava-cache/SKILL.md) | swap Guava cache (`com.google.common.cache.*`) for Caffeine (`com.github.benmanes.caffeine.cache.*`) — pom dependency + imports + API call sites | low | ready | analyzer | guided | | `unclosed-resources` | close `ResourceResolver` / `Session` / streams via try-with-resources | high | planned | - | - | | `thread-lock-contention` | replace coarse `synchronized` / synchronized collections on shared state with concurrent types | high | planned | - | - | | `heavy-model-init` | move heavy work (I/O, queries) out of `@PostConstruct` / Sling Model init | medium | planned | - | - | diff --git a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java index 4fe1f7ebd..97076a63c 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java +++ b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java @@ -2,6 +2,7 @@ import analyzer.detectors.AssetManager; import analyzer.detectors.EventMigration; +import analyzer.detectors.GuavaCache; import analyzer.detectors.InjectInSlingModel; import analyzer.detectors.OutboundCallTimeouts; import analyzer.detectors.OutdatedDependencies; @@ -28,7 +29,8 @@ public static List all() { new AssetManager(), new OutboundCallTimeouts(), new UnboundedQuery(), - new RemoveDeprecatedApi() + new RemoveDeprecatedApi(), + new GuavaCache() )); } } diff --git a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java new file mode 100644 index 000000000..41e6aa971 --- /dev/null +++ b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java @@ -0,0 +1,41 @@ +package analyzer.detectors; + +import analyzer.Corpus; +import analyzer.Detector; +import analyzer.Finding; +import analyzer.JavaUnit; +import com.sun.source.tree.ImportTree; + +import java.util.List; + +/** + * guava-cache — a bundle importing {@code com.google.common.cache.*} (Guava's in-process cache + * API). On AEM as a Cloud Service the supported in-process cache library is Caffeine + * ({@code com.github.benmanes.caffeine.cache.*}); Guava cache is flagged because Guava is shrinking + * in the CS uber-jar and relying on {@code com.google.common.cache} from a third-party classloader + * is unstable. The remediation is a near-1:1 swap (pom dependency + imports + a few API call sites). + * + *

Parse-level and import-anchored: it flags each {@code import com.google.common.cache.…} + * (explicit type or the package wildcard {@code .*}). Other Guava packages + * ({@code com.google.common.collect} / {@code .base} / …) are out of scope, and look-alike cache + * classes from other packages (e.g. {@code io.micrometer…cache.GuavaCacheMetrics}) are not matched + * because the prefix is exact — avoiding the known BPA false positive. + */ +public final class GuavaCache implements Detector { + + public String pattern() { return "guava-cache"; } + public boolean needsPoms() { return false; } // Java-only detector (import-anchored) + + private static final String GUAVA_CACHE_PKG = "com.google.common.cache."; + + public void detect(Corpus c, List out, List warnings) { + for (JavaUnit u : c.java) { + for (ImportTree imp : u.cu.getImports()) { + String q = imp.getQualifiedIdentifier().toString(); + if (q.startsWith(GUAVA_CACHE_PKG)) { + out.add(new Finding(pattern(), u.rel, u.lineOf(imp), u.snippetOf(imp))); + } + } + } + } +} diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java new file mode 100644 index 000000000..d2a5a8771 --- /dev/null +++ b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java @@ -0,0 +1,23 @@ +package fixtures; + +import com.github.benmanes.caffeine.cache.Caffeine; +import com.github.benmanes.caffeine.cache.LoadingCache; + +import java.time.Duration; + +/** Clean: already migrated to Caffeine — must NOT be flagged by guava-cache. */ +public class CleanCaffeineCache { + + private final LoadingCache titles = Caffeine.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(Duration.ofMinutes(10)) + .build(this::resolve); + + public String title(String id) { + return titles.get(id); + } + + private String resolve(String id) { + return id.toUpperCase(); + } +} diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java new file mode 100644 index 000000000..506601db8 --- /dev/null +++ b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java @@ -0,0 +1,29 @@ +package fixtures; + +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; + +import java.util.concurrent.TimeUnit; + +/** Antipattern: imports com.google.common.cache.* — must be flagged by guava-cache. */ +public class LegacyGuavaCache { + + private final LoadingCache titles = CacheBuilder.newBuilder() + .maximumSize(10_000) + .expireAfterWrite(10, TimeUnit.MINUTES) + .build(new CacheLoader() { + @Override + public String load(String id) { + return resolve(id); + } + }); + + public String title(String id) { + return titles.getUnchecked(id); + } + + private String resolve(String id) { + return id.toUpperCase(); + } +} diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java new file mode 100644 index 000000000..f836ddf5a --- /dev/null +++ b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java @@ -0,0 +1,15 @@ +package fixtures; + +import io.micrometer.core.instrument.binder.cache.GuavaCacheMetrics; + +/** + * Negative / false-positive guard: a look-alike "GuavaCache" class from a different package + * (Micrometer's metrics binder, not com.google.common.cache.*). Must NOT be flagged — the BPA + * report's known guava false positive. + */ +public class MicrometerGuavaMetrics { + + public Class binder() { + return GuavaCacheMetrics.class; + } +} diff --git a/plugins/aem/cloud-service/test/code-assessment/run-tests.sh b/plugins/aem/cloud-service/test/code-assessment/run-tests.sh index 77090d689..3b14fefa0 100755 --- a/plugins/aem/cloud-service/test/code-assessment/run-tests.sh +++ b/plugins/aem/cloud-service/test/code-assessment/run-tests.sh @@ -251,6 +251,12 @@ assert_contains "warning for missing cache" "$OUT" 'deprecated-api-rules- assert_absent "no findings when cache is missing" "$OUT" '"pattern":"remove-deprecated-api"' rm -f "$RULES_TSV" +echo "[guava-cache] com.google.common.cache.* imports flagged; caffeine + micrometer look-alike not flagged" +OUT="$(run "$FIX/guava-cache")" +assert_contains "pattern present" "$OUT" '"pattern":"guava-cache"' +assert_contains "LegacyGuavaCache flagged" "$OUT" 'LegacyGuavaCache.java' +assert_absent "CleanCaffeineCache not flagged" "$OUT" 'CleanCaffeineCache.java' +assert_absent "MicrometerGuavaMetrics not flagged" "$OUT" 'MicrometerGuavaMetrics.java' echo "----" echo "PASS=$PASS FAIL=$FAIL" From 48efad56b11f3c5a04322d738083c131ab297a78 Mon Sep 17 00:00:00 2001 From: bharat941 Date: Fri, 4 Sep 2026 15:50:57 +0530 Subject: [PATCH 2/4] fix(aem-cloud-service): move guava-cache from code-assessment to migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Guava cache usage doesn't occur in native AEMaaCS code, only in code carried over from legacy AEM — so per review feedback this belongs to the migration skill's domain, not code-assessment's. - Remove the guava-cache expert skill, analyzer detector, and fixtures from code-assessment; revert its Registry/SKILL.md/patterns.md/run-tests.sh hooks. - Add migration/references/guava-cache.md (BPA-driven reference, no dedicated pattern guide — same shape as htlLint). - Wire guavaCache as a BPA-only pattern (subtype com.google.common.cache) into bpa-local-parser.js, unified-collection-reader.js, and runbook-generator.js's PATTERN_META — one finding per file, not per import, since BPA already reports at file granularity. No analyzer/content-scan fallback is added; with no BPA source the pattern surfaces via the existing LLM-scan tier. - Drop internal "avoids the known BPA false positive" language; BPA is now the stated source of truth for this pattern. Co-Authored-By: Claude Sonnet 5 --- .../skills/code-assessment/SKILL.md | 1 - .../code-assessment/guava-cache/SKILL.md | 63 ------------------- .../code-assessment/references/patterns.md | 1 - .../scripts/analyzer/Registry.java | 4 +- .../analyzer/detectors/GuavaCache.java | 41 ------------ .../cloud-service/skills/migration/SKILL.md | 9 ++- .../references/guava-cache.md} | 63 ++++++++++++++----- .../migration/scripts/bpa-local-parser.js | 51 ++++++++++++++- .../migration/scripts/runbook-generator.js | 26 ++++++-- .../scripts/runbook-generator.test.js | 12 +++- .../scripts/unified-collection-reader.js | 37 ++++++++++- .../guava-cache/CleanCaffeineCache.java | 23 ------- .../guava-cache/LegacyGuavaCache.java | 29 --------- .../guava-cache/MicrometerGuavaMetrics.java | 15 ----- .../test/code-assessment/run-tests.sh | 6 -- 15 files changed, 169 insertions(+), 212 deletions(-) delete mode 100644 plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md delete mode 100644 plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java rename plugins/aem/cloud-service/skills/{code-assessment/guava-cache/recipe.md => migration/references/guava-cache.md} (61%) delete mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java delete mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java delete mode 100644 plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java diff --git a/plugins/aem/cloud-service/skills/code-assessment/SKILL.md b/plugins/aem/cloud-service/skills/code-assessment/SKILL.md index 4a72e39be..020dbb6f5 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/SKILL.md +++ b/plugins/aem/cloud-service/skills/code-assessment/SKILL.md @@ -66,7 +66,6 @@ Route the request to one expert skill. Two pattern families share this skill: | `com.day.cq.replication.Replicator`, `org.apache.sling.replication.*`, "publish/preview activation" | [`replication/`](replication/SKILL.md) | `replication` | | `javax.jcr.observation.EventListener`, `org.osgi.service.event.EventHandler` on non-resource topics (replication, workflow, custom) | [`event-migration/`](event-migration/SKILL.md) | `eventListener` / `eventHandler` | | `com.day.cq.dam.api.AssetManager` create/upload/delete APIs, `createAssetForBinary`, `removeAssetForBinary` | [`asset-manager/`](asset-manager/SKILL.md) | `assetApi` | -| `import com.google.common.cache.*` (`CacheBuilder`, `LoadingCache`, `CacheLoader`), "guava cache", "swap guava for caffeine" | [`guava-cache/`](guava-cache/SKILL.md) | `guavaCache` | | HTL build warning `data-sly-test: redundant constant value comparison` | [`references/data-sly-test-redundant-constant.md`](references/data-sly-test-redundant-constant.md) | `htlLint` (reference, no expert skill subdirectory) | **Broad / correctness-review asks** ("check my Sling Models are implemented correctly", "review my code", "is my AEM project healthy", "assess this project") are not a single pattern: run the runbook in `discover` mode with intent `report` — the analyzer runs every detector and the report covers all built patterns, explicitly noting aspects not yet supported. Only narrow to one pattern when the user targets a specific fix. diff --git a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md b/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md deleted file mode 100644 index 4f2138cff..000000000 --- a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/SKILL.md +++ /dev/null @@ -1,63 +0,0 @@ ---- -name: guava-cache -description: AEM Cloud Service expert skill for the Guava cache → Caffeine swap. Covers bundles importing com.google.common.cache.* (Cache, CacheBuilder, LoadingCache, CacheLoader, RemovalListener). Classification, the near-1:1 Caffeine API mapping, pom dependency swap, import + call-site edits (getUnchecked→get, Callable→Function, RemovalNotification→3-arg), review checklist, and test generation. Routes detail to recipe.md. -license: Apache-2.0 ---- - -# Guava cache → Caffeine — AEM as a Cloud Service - -> This pattern is executed by the code-assessment runbook — follow [`../references/runbook.md`](../references/runbook.md) for the full flow (preflight → plan → apply → verify, run log). This skill supplies the detection + recipe the runbook applies. - -## Overview - -On AEM as a Cloud Service the supported in-process cache library is **Caffeine** (`com.github.benmanes.caffeine.cache.*`). Bundles importing `com.google.common.cache.*` are flagged because Guava is shrinking in the CS uber-jar and relying on Guava's cache from a third-party classloader is unstable. Caffeine is the recommended successor (same author as Guava cache) and its API is intentionally near-identical, so the swap is mechanical with a few well-known call-site renames. - -## Classification — confirm this pattern applies - -A file is in scope when it imports `com.google.common.cache.*` — `Cache`, `CacheBuilder`, `LoadingCache`, `CacheLoader`, `RemovalListener`, or `RemovalNotification`. - -1. **Bundle uses Guava cache** (import + real usage) → apply the full recipe: **C1 (pom)** + **C2 (imports)** + **C3 (builder / API call sites)**. -2. **Leftover import, no real usage** → just remove the dead `com.google.common.cache.*` import; no Caffeine dependency needed. -3. **Cache plus unrelated Guava utilities** (`com.google.common.collect.*`, `com.google.common.base.*`) → only the cache portion is in scope; leave other Guava usages alone unless the user asks. Keep the `guava` dependency if other code still imports `com.google.common.*`. - -**Not in scope (no false positive):** look-alike cache classes from other packages — e.g. `io.micrometer.core.instrument.binder.cache.GuavaCacheMetrics` — are not `com.google.common.cache.*` and are not flagged. - -## Discovery - -Detection is performed by the analyzer ([`../scripts/analyze.sh`](../scripts/README.md)), run by the runbook: - -```bash -bash ../scripts/analyze.sh --pattern guava-cache -``` - -**Match criteria (what the detector flags):** each `import com.google.common.cache.…` (explicit type or the package wildcard `.*`) in a parsed Java file, emitted with the import line and snippet. The match is an exact package-prefix on the import — Java-only, parse-level, import-anchored; no classpath or type resolution. - -## Resolution contract - -**self-evident** — the Guava → Caffeine API mapping is fixed (see [recipe.md](recipe.md)); no user input is required to plan the edit. The only judgment is the Caffeine version: pin it to the AEM CS SDK BOM (default `3.1.8`). - -## Review checklist - -- [ ] No `import com.google.common.cache.*` remains in changed files. -- [ ] No `CacheBuilder.newBuilder()` remains; all builders use `Caffeine.newBuilder()`. -- [ ] No `LoadingCache.getUnchecked(...)` remains; replaced with `.get(...)`. -- [ ] No `cache.get(key, Callable)` remains; the `Callable` is a `Function` (lambda). -- [ ] `RemovalListener` callbacks use the `(key, value, cause)` signature, not `RemovalNotification`. -- [ ] `Caffeine` is on the bundle's `pom.xml` with `provided`, version pinned to the AEM CS SDK BOM. -- [ ] `mvn clean install` passes; **aemanalyser** reports no `com.google.common.cache` API leaks. -- [ ] Guava dependency kept only if non-cache `com.google.common.*` usage remains. - -## Common pitfalls - -- **Embedding Caffeine** — use `provided`; Caffeine is supplied by the CS runtime, never embed it in the bundle. -- **Removing Guava too eagerly** — if other code still imports `com.google.common.collect/base`, keep the dependency and add Caffeine alongside. -- **`getUnchecked` left in place** — Caffeine has no `getUnchecked`; `LoadingCache.get(key)` already throws unchecked. -- **`Callable` vs `Function`** — `cache.get(key, …)` takes a `Function` in Caffeine, not a `Callable`. - -## Recipe - -Read [recipe.md](recipe.md) in full before editing: input contract, the C1/C2/C3 edits, the API mapping table, unlocatable / skip reasons, before/after examples, and test generation. - -## Handoff - -The skill never commits. See [`../references/git-workflow.md`](../references/git-workflow.md) for git vs in-place handoff and the suggested commit message. diff --git a/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md b/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md index bbac1e957..2ca9a413d 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md +++ b/plugins/aem/cloud-service/skills/code-assessment/references/patterns.md @@ -39,7 +39,6 @@ Adding a pattern: add a row here, then build the expert skill from | [`asset-manager`](../asset-manager/SKILL.md) | migrate DAM `AssetManager` create/upload via Direct Binary Access (`@adobe/aem-upload`) and delete via in-JVM `resolver.delete()` + `commit()` or HTTP Assets API; removes `createAssetForBinary` / `getAssetForBinary` / `removeAssetForBinary` (not available on CS) | high | ready | analyzer | guided | | [`outbound-call-timeouts`](../outbound-call-timeouts/SKILL.md) | add connect/read/socket timeouts to outbound HTTP client construction (Apache HttpClient, OkHttp, JDK HttpClient) | high | ready | analyzer | mechanical | | [`unbounded-query`](../unbounded-query/SKILL.md) | bound or escalate an explicitly-unbounded query (`p.limit=-1` predicate / `setLimit(-1)`) — safe-cap where provable, else flag for pagination | high | ready | analyzer | guided | -| [`guava-cache`](../guava-cache/SKILL.md) | swap Guava cache (`com.google.common.cache.*`) for Caffeine (`com.github.benmanes.caffeine.cache.*`) — pom dependency + imports + API call sites | low | ready | analyzer | guided | | `unclosed-resources` | close `ResourceResolver` / `Session` / streams via try-with-resources | high | planned | - | - | | `thread-lock-contention` | replace coarse `synchronized` / synchronized collections on shared state with concurrent types | high | planned | - | - | | `heavy-model-init` | move heavy work (I/O, queries) out of `@PostConstruct` / Sling Model init | medium | planned | - | - | diff --git a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java index 97076a63c..4fe1f7ebd 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java +++ b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/Registry.java @@ -2,7 +2,6 @@ import analyzer.detectors.AssetManager; import analyzer.detectors.EventMigration; -import analyzer.detectors.GuavaCache; import analyzer.detectors.InjectInSlingModel; import analyzer.detectors.OutboundCallTimeouts; import analyzer.detectors.OutdatedDependencies; @@ -29,8 +28,7 @@ public static List all() { new AssetManager(), new OutboundCallTimeouts(), new UnboundedQuery(), - new RemoveDeprecatedApi(), - new GuavaCache() + new RemoveDeprecatedApi() )); } } diff --git a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java b/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java deleted file mode 100644 index 41e6aa971..000000000 --- a/plugins/aem/cloud-service/skills/code-assessment/scripts/analyzer/detectors/GuavaCache.java +++ /dev/null @@ -1,41 +0,0 @@ -package analyzer.detectors; - -import analyzer.Corpus; -import analyzer.Detector; -import analyzer.Finding; -import analyzer.JavaUnit; -import com.sun.source.tree.ImportTree; - -import java.util.List; - -/** - * guava-cache — a bundle importing {@code com.google.common.cache.*} (Guava's in-process cache - * API). On AEM as a Cloud Service the supported in-process cache library is Caffeine - * ({@code com.github.benmanes.caffeine.cache.*}); Guava cache is flagged because Guava is shrinking - * in the CS uber-jar and relying on {@code com.google.common.cache} from a third-party classloader - * is unstable. The remediation is a near-1:1 swap (pom dependency + imports + a few API call sites). - * - *

Parse-level and import-anchored: it flags each {@code import com.google.common.cache.…} - * (explicit type or the package wildcard {@code .*}). Other Guava packages - * ({@code com.google.common.collect} / {@code .base} / …) are out of scope, and look-alike cache - * classes from other packages (e.g. {@code io.micrometer…cache.GuavaCacheMetrics}) are not matched - * because the prefix is exact — avoiding the known BPA false positive. - */ -public final class GuavaCache implements Detector { - - public String pattern() { return "guava-cache"; } - public boolean needsPoms() { return false; } // Java-only detector (import-anchored) - - private static final String GUAVA_CACHE_PKG = "com.google.common.cache."; - - public void detect(Corpus c, List out, List warnings) { - for (JavaUnit u : c.java) { - for (ImportTree imp : u.cu.getImports()) { - String q = imp.getQualifiedIdentifier().toString(); - if (q.startsWith(GUAVA_CACHE_PKG)) { - out.add(new Finding(pattern(), u.rel, u.lineOf(imp), u.snippetOf(imp))); - } - } - } - } -} diff --git a/plugins/aem/cloud-service/skills/migration/SKILL.md b/plugins/aem/cloud-service/skills/migration/SKILL.md index f3b3017d4..715626fc2 100644 --- a/plugins/aem/cloud-service/skills/migration/SKILL.md +++ b/plugins/aem/cloud-service/skills/migration/SKILL.md @@ -1,6 +1,6 @@ --- name: migration -description: Migrates legacy AEM (6.x, AMS, on-prem) to AEM as a Cloud Service using BPA CSV/cache, CAM/MCP discovery, and a one-pattern-per-session workflow. Use to review/scan a project for AEMaaCS migration (generates a read-only migration-runbook.md covering all patterns via per-pattern detection strategies), for BPA/CAM findings, Cloud Service blockers, or fixes for scheduler, ResourceChangeListener, replication, EventListener, OSGi EventHandler, DAM AssetManager, HTL data-sly-test lint, Classic UI dialog migration (lui — ExtJS/Coral 2 → Coral 3), Custom Design Widgets (cdw), and static→editable template modernization. OSGi configs → Cloud Manager — scan ui.config/.cfg.json for secrets and $[secret:]/$[env:] placeholders. After discovery, migration hands off each (pattern, file) pair to the code-assessment skill for the pattern guides and shared references; template modernization and legacy UI (dialog/CDW) follow references/ modules. +description: Migrates legacy AEM (6.x, AMS, on-prem) to AEM as a Cloud Service using BPA CSV/cache, CAM/MCP discovery, and a one-pattern-per-session workflow. Use to review/scan a project for AEMaaCS migration (generates a read-only migration-runbook.md covering all patterns via per-pattern detection strategies), for BPA/CAM findings, Cloud Service blockers, or fixes for scheduler, ResourceChangeListener, replication, EventListener, OSGi EventHandler, DAM AssetManager, HTL data-sly-test lint, Classic UI dialog migration (lui — ExtJS/Coral 2 → Coral 3), Custom Design Widgets (cdw), Guava cache → Caffeine swaps (guavaCache), and static→editable template modernization. OSGi configs → Cloud Manager — scan ui.config/.cfg.json for secrets and $[secret:]/$[env:] placeholders. After discovery, migration hands off each (pattern, file) pair to the code-assessment skill for the pattern guides and shared references; template modernization, legacy UI (dialog/CDW), and Guava cache follow references/ modules. license: Apache-2.0 --- @@ -27,6 +27,7 @@ This skill drives the **migration workflow**: BPA data, CAM/MCP, **one pattern p | **Template modernization** | *"**Migrate my static templates to editable templates and generate Modernize Tools rules.**"* / *"Create editable templates from my static templates."* / *"Generate AEM Modernize Tools structure/component/policy rules."* | Agent **auto-reads** [references/template-modernization/template-modernization-context.md](references/template-modernization/template-modernization-context.md) (shared discovery + structured context), produces a **per-template plan table**, then executes the plan using [editable-template-creation.md](references/template-modernization/editable-template-creation.md) and [aem-modernization.md](references/template-modernization/aem-modernization.md), and validates via [template-modernization-validation.md](references/template-modernization/template-modernization-validation.md). No BPA pattern id. | | **Dialog migration** | *"Convert my Classic UI / ExtJS dialogs to Touch UI."* / *"Upgrade Coral 2 dialogs to Coral 3."* / *"Fix LUI dialog findings."* | Agent reads [references/legacy-ui/dialog/context.md](references/legacy-ui/dialog/context.md) — filters BPA LUI to dialog sub-types, converts via [extjs-to-coral3.md](references/legacy-ui/dialog/extjs-to-coral3.md) or [coral2-to-coral3.md](references/legacy-ui/dialog/coral2-to-coral3.md), validates via [validation.md](references/legacy-ui/dialog/validation.md). BPA pattern id: `lui`. | | **Custom widget migration** | *"Fix my CDW findings."* / *"Migrate custom ExtJS widgets to Coral 3."* | Agent reads [references/legacy-ui/cdw/context.md](references/legacy-ui/cdw/context.md) — inventories xtypes, maps or scaffolds Granite UI components via [conversion.md](references/legacy-ui/cdw/conversion.md), validates via [validation.md](references/legacy-ui/cdw/validation.md). BPA pattern id: `cdw`. Run CDW before dialog migration when both are needed. | +| **Guava cache warnings** | *"Fix **guavaCache** findings using BPA CSV."* / *"Swap Guava cache for Caffeine."* | Agent reads [references/guava-cache.md](references/guava-cache.md) — BPA is the source of truth (subtype `com.google.common.cache`); one finding per file, not per import. BPA pattern id: `guavaCache`. Not a `code-assessment` pattern — Guava cache usage only occurs in pre-migration code, never native AEMaaCS code. | **Starter prompts (copy-paste):** @@ -40,6 +41,7 @@ This skill drives the **migration workflow**: BPA data, CAM/MCP, **one pattern p - *"Fix LUI dialog findings using BPA CSV at `./reports/bpa.csv`."* - *"Migrate custom ExtJS widgets (CDW findings) from CAM."* - *"Fix all Classic UI and custom widget findings — CDW first, then dialogs."* +- *"Fix **guavaCache** findings using BPA CSV at `./reports/bpa.csv`."* ## Path convention (Adobe Skills monorepo) @@ -75,6 +77,7 @@ Applies to **finding and editing the user's AEM project** (Java, bundles, config - `eventListener` / `eventHandler` → **`{code-assessment}/event-migration/SKILL.md`** *(pattern guide — both JCR and OSGi Event Admin paths)* - `assetApi` → **`{code-assessment}/asset-manager/SKILL.md`** *(pattern guide)* - `htlLint` → **`{code-assessment}/references/data-sly-test-redundant-constant.md`** *(reference — HTL lint is a single shared reference, not a dedicated pattern guide)* + - `guavaCache` → **[references/guava-cache.md](references/guava-cache.md)** *(reference — Guava cache → Caffeine swap; lives under `migration` only, not `code-assessment`, since Guava cache usage does not occur in native AEMaaCS code, only in code carried over from legacy AEM)* 3. When code uses SCR, `ResourceResolver`, or console logging, read **`{code-assessment}/references/scr-to-osgi-ds.md`** and **`{code-assessment}/references/resource-resolver-logging.md`** (or the hub **`{code-assessment}/references/aem-cloud-service-pattern-prerequisites.md`**). Do not transform **Java or HTL** until the pattern guide (or reference) is read (branch B). Branch A does not require `{code-assessment}` pattern guidance. @@ -101,6 +104,7 @@ Do not transform **Java or HTL** until the pattern guide (or reference) is read - Migrate legacy AEM Java toward **Cloud Service–compatible** patterns (scheduler, ResourceChangeListener, replication, EventListener/EventHandler, AssetManager) - Fix **HTL (Sightly)** lint warnings (`data-sly-test: redundant constant value comparison`) +- Swap **Guava cache** (`com.google.common.cache.*`) for **Caffeine** (`guavaCache`) - **OSGi → Cloud Manager** secret/variable externalization (Branch A), **Template Modernization** (Branch C), **Legacy UI** dialog/CDW migration (Branch D) - Drive work from **BPA** (CSV or cached collection) or **CAM via MCP**, **one pattern per session** @@ -222,8 +226,9 @@ The runbook covers **every pattern the migration skill can address**. Each patte | `htlLint` | `html-scan` | heuristic regex scan of `.html` (pure Node — no `rg` binary needed) | | `osgiConfig` | `config-scan` | heuristic scan of OSGi config files for secret-looking keys / `$[secret:]`/`$[env:]` placeholders — **key names + locations only, never secret values** | | `lui`, `cdw`, `templateModernization` | BPA `cascade` → `content-scan` fallback | When a BPA CSV/CAM source is present, these come from BPA (subtypes `custom.classic.widget`; `legacy.dialog.classic`/`.coral2`; `legacy.static.template` + `custom.static.template`). With no BPA source, a heuristic `.content.xml` scan is the fallback — for `templateModernization` it walks `apps//templates/**` at **any depth** (nested/grouped templates included) and classifies each static template as `custom.static.template` or `legacy.static.template` from its page-component resource type, so the custom-vs-legacy distinction survives even without a BPA report. Sample prompts route to **Branch D** (legacy-ui) / **Branch C** (templates), not code-assessment | +| `guavaCache` | `bpa-only` (no analyzer, no content-scan) | BPA is the **sole** source of truth (subtype `com.google.common.cache`), one finding per file. With no BPA source, `guavaCache` has no deterministic fallback and surfaces under **Tier 4 — LLM scan**: the agent greps `.java` files for `import com.google.common.cache` per [references/guava-cache.md](references/guava-cache.md) and tags the result `confidence: llm`. There is deliberately no compiled analyzer detector for this pattern — it does not run inside `code-assessment`'s own discovery. | -`htlLint`, `osgiConfig`, and the content-scan **fallback** for `lui`/`cdw`/`templateModernization` are **heuristic** (tagged `confidence: heuristic` in the cache) — candidate matches, not compiler-validated. BPA-sourced `lui`/`cdw`/`templateModernization`/`replication` findings are authoritative. Out of scope: `inject-in-sling-model` and `outdated-dependencies` (those belong to code-assessment's own runbook, not migration). +`htlLint`, `osgiConfig`, and the content-scan **fallback** for `lui`/`cdw`/`templateModernization` are **heuristic** (tagged `confidence: heuristic` in the cache) — candidate matches, not compiler-validated. BPA-sourced `lui`/`cdw`/`templateModernization`/`replication`/`guavaCache` findings are authoritative. Out of scope: `inject-in-sling-model` and `outdated-dependencies` (those belong to code-assessment's own runbook, not migration). **BPA is the source of truth when a report is available.** `lui`/`cdw`/`templateModernization`/`replication` are read from the BPA CSV/CAM (the parser now extracts these subtypes and excludes `_COUNT_*`/`_STAT` summary rows), so the runbook counts match your BPA report's LUI-dialog / CDW / static-template / REP tallies. `lui` keeps only the dialog sub-types (`legacy.custom.component` → create-component; `legacy.static.template` is counted under `templateModernization`). The `.content.xml` scan is only the fallback when no BPA source is present — and it can **undercount** relative to BPA when the flagged legacy nodes live in packages (e.g. acs-commons) not in the project source. `replication`: BPA `replication.agent` findings when a report is present, else the analyzer detects `Replicator` usage from source. diff --git a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md b/plugins/aem/cloud-service/skills/migration/references/guava-cache.md similarity index 61% rename from plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md rename to plugins/aem/cloud-service/skills/migration/references/guava-cache.md index 81874ec42..bfb083dc5 100644 --- a/plugins/aem/cloud-service/skills/code-assessment/guava-cache/recipe.md +++ b/plugins/aem/cloud-service/skills/migration/references/guava-cache.md @@ -1,18 +1,32 @@ -# Recipe — Guava cache → Caffeine +# Guava cache → Caffeine on AEM as a Cloud Service -> Read this fully before editing. Control plane: [SKILL.md](SKILL.md). +BPA pattern id: **`guavaCache`**. Not a Cloud-Service-native code-quality issue — it only shows up in code carried over from a pre-cloud (legacy AEM 6.x / AMS / on-prem) codebase, so this is a migration reference, not a `code-assessment` pattern. -## Input contract +## Why it's flagged -Per finding, regardless of how it was obtained: +On AEM as a Cloud Service the supported in-process cache library is **Caffeine** (`com.github.benmanes.caffeine.cache.*`). Bundles importing `com.google.common.cache.*` are flagged because Guava is shrinking in the CS uber-jar and relying on Guava's cache from a third-party classloader is unstable. Caffeine is the recommended successor (same author as Guava cache) and its API is intentionally near-identical, so the swap is mechanical with a few well-known call-site renames. -| Field | Example | Source | -|---|---|---| -| `file` | `core/.../UserCache.java` | Repo-relative path to the flagged Java file | -| `line` | `7` | Line of the `com.google.common.cache.*` import | -| `snippet` | `import com.google.common.cache.CacheBuilder;` | The flagged import | +## Discovery — BPA is the source of truth -The fix parameters are **self-evident** — the Guava → Caffeine mapping below is fixed. The only choice is the Caffeine version: pin to the AEM CS SDK BOM (default `3.1.8`). +Findings come from **`getBpaFindings('guavaCache', …)`** (BPA CSV column `subtype` = `com.google.common.cache`). When no BPA/CAM source is available, scan the workspace's `.java` files for `import com.google.common.cache.…` — treat this as a manual, unconfirmed lead per file, not a substitute for BPA. + +Group by **file**, not by import: a file with multiple `com.google.common.cache.*` imports is one finding, one migration unit — apply the full recipe to that file once. + +BPA gives only a `file` (no `line`/`snippet`) — there is no analyzer detector to resolve those, unlike the `code-assessment` cascade patterns. Open the file directly and locate the `com.google.common.cache.*` imports yourself before editing; do not look for a `guava-cache` entry in the analyzer. + +## Classification + +A file is in scope when it imports `com.google.common.cache.*` — `Cache`, `CacheBuilder`, `LoadingCache`, `CacheLoader`, `RemovalListener`, or `RemovalNotification`. + +1. **Bundle uses Guava cache** (import + real usage) → apply the full recipe: **C1 (pom)** + **C2 (imports)** + **C3 (builder / API call sites)**. +2. **Leftover import, no real usage** → just remove the dead `com.google.common.cache.*` import; no Caffeine dependency needed. +3. **Cache plus unrelated Guava utilities** (`com.google.common.collect.*`, `com.google.common.base.*`) → only the cache portion is in scope; leave other Guava usages alone unless the user asks. Keep the `guava` dependency if other code still imports `com.google.common.*`. + +**Not in scope:** look-alike cache classes from other packages — e.g. `io.micrometer.core.instrument.binder.cache.GuavaCacheMetrics` — are not `com.google.common.cache.*` and should not be flagged. If a manual scan flags one of these, treat it as a scan error and skip it; it is not a Guava cache usage. + +## Resolution contract + +**Self-evident** — the Guava → Caffeine API mapping below is fixed; no user input is required to plan the edit. The only judgment call is the Caffeine version: pin it to the AEM CS SDK BOM (default `3.1.8`). ## API mapping (Guava → Caffeine) @@ -128,11 +142,6 @@ RemovalListener rl = (key, value, cause) -> log.info("evicted {} cause={}", key, cause); ``` -## Unlocatable / skip - -- `import-not-found: com.google.common.cache.* not present in ` — the flagged import is no longer there (already migrated). Record `skipped`. -- `guava-still-required: non-cache com.google.common.* usage in ` — only remove the cache imports; keep the Guava dependency. Record as a partial apply note, not a skip. - ## Editing strategy Surgical, formatting-preserving text edits — no reformatting / re-serialization: @@ -144,6 +153,29 @@ Surgical, formatting-preserving text edits — no reformatting / re-serializatio Anchor each replace on the smallest unique substring so unrelated identical text is not touched. +## Unlocatable / skip + +- `import-not-found: com.google.common.cache.* not present in ` — the flagged import is no longer there (already migrated). Record `skipped`. +- `guava-still-required: non-cache com.google.common.* usage in ` — only remove the cache imports; keep the Guava dependency. Record as a partial apply note, not a skip. + +## Review checklist + +- [ ] No `import com.google.common.cache.*` remains in changed files. +- [ ] No `CacheBuilder.newBuilder()` remains; all builders use `Caffeine.newBuilder()`. +- [ ] No `LoadingCache.getUnchecked(...)` remains; replaced with `.get(...)`. +- [ ] No `cache.get(key, Callable)` remains; the `Callable` is a `Function` (lambda). +- [ ] `RemovalListener` callbacks use the `(key, value, cause)` signature, not `RemovalNotification`. +- [ ] `Caffeine` is on the bundle's `pom.xml` with `provided`, version pinned to the AEM CS SDK BOM. +- [ ] `mvn clean install` passes. +- [ ] Guava dependency kept only if non-cache `com.google.common.*` usage remains. + +## Common pitfalls + +- **Embedding Caffeine** — use `provided`; Caffeine is supplied by the CS runtime, never embed it in the bundle. +- **Removing Guava too eagerly** — if other code still imports `com.google.common.collect/base`, keep the dependency and add Caffeine alongside. +- **`getUnchecked` left in place** — Caffeine has no `getUnchecked`; `LoadingCache.get(key)` already throws unchecked. +- **`Callable` vs `Function`** — `cache.get(key, …)` takes a `Function` in Caffeine, not a `Callable`. + ## Test generation After the swap, generate a JUnit test that confirms cache behaviour is preserved — `getIfPresent` returns `null` for unknown keys, `cache.get(key, Function)` computes and caches, `invalidate(key)` removes the entry, and `LoadingCache.get(key)` does not throw checked exceptions. One test class per production class changed, suffix `Test`, under `src/test/java/…`. @@ -189,5 +221,4 @@ public class UserCacheTest { ## See also -- [`../references/aem-cloud-service-pattern-prerequisites.md`](../references/aem-cloud-service-pattern-prerequisites.md) — SCR → DS, service-user resolvers, SLF4J. - Caffeine wiki: — behaviour differences (async loading, weight-based eviction) beyond this near-1:1 swap. diff --git a/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js b/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js index 349cda1bb..8ae5ade5a 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js @@ -20,14 +20,16 @@ const path = require('path'); const PATTERN_TO_SUBTYPE = { scheduler: "sling.commons.scheduler", assetApi: "unsupported.asset.api", + guavaCache: "com.google.common.cache", }; // CSV subtype to pattern mapping (based on actual CSV structure) const CSV_SUBTYPE_TO_PATTERN = { "unsupported.asset.api": "assetApi", - "javax.jcr.observation.EventListener": "eventListener", + "javax.jcr.observation.EventListener": "eventListener", "org.apache.sling.api.resource.observation.ResourceChangeListener": "resourceChangeListener", - "org.osgi.service.event.EventHandler": "eventHandler" + "org.osgi.service.event.EventHandler": "eventHandler", + "com.google.common.cache": "guavaCache" }; // Known scheduler identifier @@ -420,6 +422,36 @@ function processEventHandlerFindings(findings) { }; } +/** + * Process Guava cache findings from CSV. One entry per file/class — a file + * with several `com.google.common.cache.*` imports is still a single + * migration unit, not one finding per import. + */ +function processGuavaCacheFindings(findings) { + const guavaCacheFindings = findings.filter(finding => + finding.subtype === 'com.google.common.cache' + ); + + const identifiers = {}; + const classNames = []; + + guavaCacheFindings.forEach(finding => { + const className = extractClassNameFromCsvFinding(finding); + if (className && !classNames.includes(className)) { + classNames.push(className); + } + }); + + if (classNames.length > 0) { + identifiers['com.google.common.cache'] = classNames; + } + + return { + subtype: 'com.google.common.cache', + identifiers: identifiers + }; +} + /** * Convert subtype to MongoDB-safe field name (matching cloud-adoption-service) */ @@ -529,6 +561,21 @@ function createUnifiedCollection(bpaData, outputDir) { console.log(`Found ${Object.values(eventHandlerCollection.identifiers).flat().length} event handler classes`); } + // Process Guava cache findings + const guavaCacheCollection = processGuavaCacheFindings(findings); + if (Object.keys(guavaCacheCollection.identifiers).length > 0) { + const mongoSafeSubtype = toMongoSafeFieldName(guavaCacheCollection.subtype); + subtypes[mongoSafeSubtype] = {}; + + Object.entries(guavaCacheCollection.identifiers).forEach(([identifier, classNames]) => { + const mongoSafeIdentifier = toMongoSafeIdentifier(identifier); + subtypes[mongoSafeSubtype][mongoSafeIdentifier] = classNames; + totalFindings += classNames.length; + }); + + console.log(`Found ${Object.values(guavaCacheCollection.identifiers).flat().length} guava cache files`); + } + // Process content / legacy-UI subtypes (cdw, lui, templates, replication). // Keys are RAW JCR paths (not MongoDB-safed) to avoid corrupting underscores. for (const subtype of CONTENT_SUBTYPES) { diff --git a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js index 4c62ee7dc..df5a3d104 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js @@ -25,6 +25,12 @@ * dialogs, custom `cq:Widget` xtypes, static templates) is the * heuristic fallback when no BPA source is available. Routed to * migration Branches D / C, not code-assessment. + * 'bpa-only' — `guavaCache`: BPA/CAM/CSV only (subtype `com.google.common.cache`), + * one finding per file. No analyzer, no content-scan — Guava + * cache usage does not occur in native AEMaaCS code, so there is + * deliberately no compiled detector for it. With no BPA source + * the pattern surfaces in `needsLlmScan` like any other + * unscanned pattern. * * `html-scan`/`config-scan`/`content-scan` (fallback) findings are tagged `confidence: 'heuristic'` in the * cache. Patterns no available strategy could scan (e.g. a cascade pattern @@ -58,11 +64,14 @@ const { runTemplateScan } = require('./template-scan-runner.js'); // 'html-scan' — pure-Node regex scan of .html (htlLint) // 'config-scan' — config-file heuristic scan (osgiConfig) // 'content-scan' — .content.xml / template scan (lui, cdw, templateModernization) +// 'bpa-only' — BPA/CAM/CSV only, no local fallback (guavaCache) // `bpaSlugs` maps a pattern to its BPA subtype(s): the Java 'cascade' patterns, -// plus replication (replication.agent) and lui/cdw/templateModernization. When a -// BPA source is present it is authoritative; html/config/content scans are the -// local fallback. (inject-in-sling-model and outdated-dependencies belong to -// code-assessment's own runbook, not the migration runbook, so they stay out.) +// plus replication (replication.agent), lui/cdw/templateModernization, and +// guavaCache (com.google.common.cache). When a BPA source is present it is +// authoritative; html/config/content scans are the local fallback for the +// patterns that have one. (inject-in-sling-model and outdated-dependencies +// belong to code-assessment's own runbook, not the migration runbook, so they +// stay out.) const PATTERN_META = { scheduler: { label: 'Scheduler', @@ -162,6 +171,15 @@ const PATTERN_META = { promptPattern: 'template modernization', sampleOverride: 'Use the migration skill: migrate my static templates to editable templates and generate the AEM Modernize Tools rewrite rules.', }, + guavaCache: { + label: 'Guava Cache → Caffeine', + severity: 'info', + strategy: 'bpa-only', + bpaSlugs: ['guavaCache'], + description: 'Bundles importing `com.google.common.cache.*` (Guava in-process cache). Migrate to Caffeine (`com.github.benmanes.caffeine.cache.*`) — a near 1:1 API swap. Not a Cloud-Service-native pattern — only found in code carried over from legacy AEM — so BPA is the sole source of truth; there is no analyzer or content-scan fallback.', + promptPattern: 'guavaCache', + sampleOverride: 'Use the migration skill: fix guavaCache findings using BPA CSV — swap Guava cache for Caffeine.', + }, }; // content-scan strategy → the runner that produces that pattern's findings. diff --git a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.test.js b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.test.js index 2ecfd74fb..ad7f47874 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.test.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.test.js @@ -28,21 +28,27 @@ function write(root, rel, content) { // ── Pattern registry ──────────────────────────────────────────────────────── -test('registry includes all 10 migration patterns with a valid strategy', () => { +test('registry includes all 11 migration patterns with a valid strategy', () => { const expected = [ 'scheduler', 'resourceChangeListener', 'event-migration', 'assetApi', 'replication', - 'htlLint', 'osgiConfig', 'lui', 'cdw', 'templateModernization', + 'htlLint', 'osgiConfig', 'lui', 'cdw', 'templateModernization', 'guavaCache', ]; assert.strictEqual(CANONICAL_PATTERNS.length, expected.length, 'no unexpected patterns'); for (const key of expected) { assert.ok(CANONICAL_PATTERNS.includes(key), `${key} in CANONICAL_PATTERNS`); assert.ok( - ['cascade', 'html-scan', 'config-scan', 'content-scan'].includes(PATTERN_META[key].strategy), + ['cascade', 'html-scan', 'config-scan', 'content-scan', 'bpa-only'].includes(PATTERN_META[key].strategy), `${key} has a valid strategy` ); } }); +test('guavaCache has no analyzer/content-scan fallback — bpaSlugs only, no heuristic flag', () => { + assert.strictEqual(PATTERN_META.guavaCache.strategy, 'bpa-only'); + assert.deepStrictEqual(PATTERN_META.guavaCache.bpaSlugs, ['guavaCache']); + assert.ok(!PATTERN_META.guavaCache.heuristic, 'guavaCache findings are BPA-authoritative, not heuristic'); +}); + test('inject-in-sling-model and outdated-dependencies stay out of scope', () => { assert.ok(!CANONICAL_PATTERNS.includes('inject-in-sling-model')); assert.ok(!CANONICAL_PATTERNS.includes('outdated-dependencies')); diff --git a/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js b/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js index 22610472c..bf8814300 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js @@ -17,8 +17,9 @@ const PATTERN_TO_SUBTYPE = { scheduler: "sling.commons.scheduler", assetApi: "unsupported.asset.api", eventListener: "javax.jcr.observation.EventListener", - resourceChangeListener: "org.apache.sling.api.resource.observation.ResourceChangeListener", - eventHandler: "org.osgi.service.event.EventHandler" + resourceChangeListener: "org.apache.sling.api.resource.observation.ResourceChangeListener", + eventHandler: "org.osgi.service.event.EventHandler", + guavaCache: "com.google.common.cache" }; // MongoDB-safe to pattern mapping @@ -27,7 +28,8 @@ const MONGO_SAFE_TO_PATTERN = { "unsupported_asset_api": "assetApi", "javax_jcr_observation_EventListener": "eventListener", "org_apache_sling_api_resource_observation_ResourceChangeListener": "resourceChangeListener", - "org_osgi_service_event_EventHandler": "eventHandler" + "org_osgi_service_event_EventHandler": "eventHandler", + "com_google_common_cache": "guavaCache" }; // Pattern → subtype(s), 1:many. Covers the Java patterns above plus the @@ -44,6 +46,7 @@ const PATTERN_TO_SUBTYPES = { lui: ["legacy.dialog.classic", "legacy.dialog.coral2", "legacy.custom.component", "legacy.static.template"], templateModernization: ["legacy.static.template", "custom.static.template"], replication: ["forward.replication", "reverse.replication"], + guavaCache: ["com.google.common.cache"], }; // Patterns whose findings are keyed by JCR path (raw keys, generic processor). @@ -322,6 +325,33 @@ function processEventHandlerFromUnified(subtypeData, targets) { return count; } +/** + * Process Guava cache data from unified collection. One target per + * file/class — BPA already reports at file granularity, not per import. + */ +function processGuavaCacheFromUnified(subtypeData, targets) { + let count = 0; + + const identifierKeys = Object.keys(subtypeData || {}).sort(); + for (const mongoSafeIdentifier of identifierKeys) { + const classNames = subtypeData[mongoSafeIdentifier] || []; + const identifier = fromMongoSafeFieldName(mongoSafeIdentifier); + + for (const className of classNames) { + count++; + targets.push(new BpaTarget( + "guavaCache", + className, + identifier, + `Imports Guava cache: ${identifier}`, + "info" + )); + } + } + + return count; +} + /** * Process a content/legacy-UI subtype whose unified data is keyed by RAW JCR * path (no MongoDB round-trip). Emits one target per finding, with the JCR path @@ -430,6 +460,7 @@ function fetchUnifiedBpaFindings(pattern = "all", collectionsDir = './unified-co eventListener: processEventListenerFromUnified, resourceChangeListener: processResourceChangeListenerFromUnified, eventHandler: processEventHandlerFromUnified, + guavaCache: processGuavaCacheFromUnified, }; // Process each pattern — a pattern may map to more than one subtype. diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java deleted file mode 100644 index d2a5a8771..000000000 --- a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/CleanCaffeineCache.java +++ /dev/null @@ -1,23 +0,0 @@ -package fixtures; - -import com.github.benmanes.caffeine.cache.Caffeine; -import com.github.benmanes.caffeine.cache.LoadingCache; - -import java.time.Duration; - -/** Clean: already migrated to Caffeine — must NOT be flagged by guava-cache. */ -public class CleanCaffeineCache { - - private final LoadingCache titles = Caffeine.newBuilder() - .maximumSize(10_000) - .expireAfterWrite(Duration.ofMinutes(10)) - .build(this::resolve); - - public String title(String id) { - return titles.get(id); - } - - private String resolve(String id) { - return id.toUpperCase(); - } -} diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java deleted file mode 100644 index 506601db8..000000000 --- a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/LegacyGuavaCache.java +++ /dev/null @@ -1,29 +0,0 @@ -package fixtures; - -import com.google.common.cache.CacheBuilder; -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; - -import java.util.concurrent.TimeUnit; - -/** Antipattern: imports com.google.common.cache.* — must be flagged by guava-cache. */ -public class LegacyGuavaCache { - - private final LoadingCache titles = CacheBuilder.newBuilder() - .maximumSize(10_000) - .expireAfterWrite(10, TimeUnit.MINUTES) - .build(new CacheLoader() { - @Override - public String load(String id) { - return resolve(id); - } - }); - - public String title(String id) { - return titles.getUnchecked(id); - } - - private String resolve(String id) { - return id.toUpperCase(); - } -} diff --git a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java b/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java deleted file mode 100644 index f836ddf5a..000000000 --- a/plugins/aem/cloud-service/test/code-assessment/fixtures/guava-cache/MicrometerGuavaMetrics.java +++ /dev/null @@ -1,15 +0,0 @@ -package fixtures; - -import io.micrometer.core.instrument.binder.cache.GuavaCacheMetrics; - -/** - * Negative / false-positive guard: a look-alike "GuavaCache" class from a different package - * (Micrometer's metrics binder, not com.google.common.cache.*). Must NOT be flagged — the BPA - * report's known guava false positive. - */ -public class MicrometerGuavaMetrics { - - public Class binder() { - return GuavaCacheMetrics.class; - } -} diff --git a/plugins/aem/cloud-service/test/code-assessment/run-tests.sh b/plugins/aem/cloud-service/test/code-assessment/run-tests.sh index 3b14fefa0..77090d689 100755 --- a/plugins/aem/cloud-service/test/code-assessment/run-tests.sh +++ b/plugins/aem/cloud-service/test/code-assessment/run-tests.sh @@ -251,12 +251,6 @@ assert_contains "warning for missing cache" "$OUT" 'deprecated-api-rules- assert_absent "no findings when cache is missing" "$OUT" '"pattern":"remove-deprecated-api"' rm -f "$RULES_TSV" -echo "[guava-cache] com.google.common.cache.* imports flagged; caffeine + micrometer look-alike not flagged" -OUT="$(run "$FIX/guava-cache")" -assert_contains "pattern present" "$OUT" '"pattern":"guava-cache"' -assert_contains "LegacyGuavaCache flagged" "$OUT" 'LegacyGuavaCache.java' -assert_absent "CleanCaffeineCache not flagged" "$OUT" 'CleanCaffeineCache.java' -assert_absent "MicrometerGuavaMetrics not flagged" "$OUT" 'MicrometerGuavaMetrics.java' echo "----" echo "PASS=$PASS FAIL=$FAIL" From c974dc0d66ec67849cf590063fe2ea13da650194 Mon Sep 17 00:00:00 2001 From: bharat941 Date: Fri, 4 Sep 2026 15:53:08 +0530 Subject: [PATCH 3/4] docs(aem-cloud-service): list guavaCache in the CAM-via-MCP pattern summary fetch-cam-bpa-findings-by-pattern's pattern enum already includes guavaCache server-side; the migration skill's own summary of that tool's patterns was missing it. Co-Authored-By: Claude Sonnet 5 --- plugins/aem/cloud-service/skills/migration/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/aem/cloud-service/skills/migration/SKILL.md b/plugins/aem/cloud-service/skills/migration/SKILL.md index 715626fc2..5b08653bb 100644 --- a/plugins/aem/cloud-service/skills/migration/SKILL.md +++ b/plugins/aem/cloud-service/skills/migration/SKILL.md @@ -139,7 +139,7 @@ neither is configured, the helper reports `no-source` and the agent asks for one ### CAM via MCP (summary) Use **`fetch-cam-bpa-findings-by-pattern`** for code-transformer pattern flows (scheduler, -assetApi, eventListener, resourceChangeListener, eventHandler, lui, cdw) and +assetApi, eventListener, resourceChangeListener, eventHandler, guavaCache, lui, cdw) and **`fetch-cam-bpa-findings-by-importance`** when the user instead asks "what are the critical/major/advisory/info findings?" (returns the latest BPA report's authoritative `_COUNT_` rows at one importance level, sorted by descending count). Either tool From 48849bedc279a490058bc280c3ea4fc0d67b7628 Mon Sep 17 00:00:00 2001 From: bharat941 Date: Fri, 4 Sep 2026 16:04:19 +0530 Subject: [PATCH 4/4] fix(aem-cloud-service): correct guavaCache BPA subtype and grouping unit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verified against a real customer BPA report (64k rows): the actual subtype is `custom.guava.cache`, not `com.google.common.cache` as assumed from the reference doc alone. More importantly, `identifier` on this subtype is a Guava-internal class (e.g. com.google.common.cache.AbstractCache) reachable on a bundle's classpath, not the customer's own class — BPA bytecode-scans Guava's own cache implementation wherever it's embedded, so one real bundle produced 1591 raw rows for two actual bundles. - Group by bundle name (extracted from the message text), not by `identifier` — dedupes to one finding per bundle instead of ~800/bundle. - Fix the subtype string everywhere it's checked/mapped. - Update the reference doc and migration/SKILL.md to describe bundle-level discovery instead of file-level, and to explain why `identifier` can't be used directly. Confirmed end-to-end against the real 64k-row report: getBpaFindings('guavaCache', ...) now correctly returns exactly the 2 real bundles instead of ~1591 bogus entries. Co-Authored-By: Claude Sonnet 5 --- .../cloud-service/skills/migration/SKILL.md | 4 +- .../migration/references/guava-cache.md | 14 +++-- .../migration/scripts/bpa-local-parser.js | 53 +++++++++++++------ .../migration/scripts/runbook-generator.js | 17 +++--- .../scripts/unified-collection-reader.js | 20 ++++--- 5 files changed, 71 insertions(+), 37 deletions(-) diff --git a/plugins/aem/cloud-service/skills/migration/SKILL.md b/plugins/aem/cloud-service/skills/migration/SKILL.md index 5b08653bb..59813aa83 100644 --- a/plugins/aem/cloud-service/skills/migration/SKILL.md +++ b/plugins/aem/cloud-service/skills/migration/SKILL.md @@ -27,7 +27,7 @@ This skill drives the **migration workflow**: BPA data, CAM/MCP, **one pattern p | **Template modernization** | *"**Migrate my static templates to editable templates and generate Modernize Tools rules.**"* / *"Create editable templates from my static templates."* / *"Generate AEM Modernize Tools structure/component/policy rules."* | Agent **auto-reads** [references/template-modernization/template-modernization-context.md](references/template-modernization/template-modernization-context.md) (shared discovery + structured context), produces a **per-template plan table**, then executes the plan using [editable-template-creation.md](references/template-modernization/editable-template-creation.md) and [aem-modernization.md](references/template-modernization/aem-modernization.md), and validates via [template-modernization-validation.md](references/template-modernization/template-modernization-validation.md). No BPA pattern id. | | **Dialog migration** | *"Convert my Classic UI / ExtJS dialogs to Touch UI."* / *"Upgrade Coral 2 dialogs to Coral 3."* / *"Fix LUI dialog findings."* | Agent reads [references/legacy-ui/dialog/context.md](references/legacy-ui/dialog/context.md) — filters BPA LUI to dialog sub-types, converts via [extjs-to-coral3.md](references/legacy-ui/dialog/extjs-to-coral3.md) or [coral2-to-coral3.md](references/legacy-ui/dialog/coral2-to-coral3.md), validates via [validation.md](references/legacy-ui/dialog/validation.md). BPA pattern id: `lui`. | | **Custom widget migration** | *"Fix my CDW findings."* / *"Migrate custom ExtJS widgets to Coral 3."* | Agent reads [references/legacy-ui/cdw/context.md](references/legacy-ui/cdw/context.md) — inventories xtypes, maps or scaffolds Granite UI components via [conversion.md](references/legacy-ui/cdw/conversion.md), validates via [validation.md](references/legacy-ui/cdw/validation.md). BPA pattern id: `cdw`. Run CDW before dialog migration when both are needed. | -| **Guava cache warnings** | *"Fix **guavaCache** findings using BPA CSV."* / *"Swap Guava cache for Caffeine."* | Agent reads [references/guava-cache.md](references/guava-cache.md) — BPA is the source of truth (subtype `com.google.common.cache`); one finding per file, not per import. BPA pattern id: `guavaCache`. Not a `code-assessment` pattern — Guava cache usage only occurs in pre-migration code, never native AEMaaCS code. | +| **Guava cache warnings** | *"Fix **guavaCache** findings using BPA CSV."* / *"Swap Guava cache for Caffeine."* | Agent reads [references/guava-cache.md](references/guava-cache.md) — BPA is the source of truth (subtype `custom.guava.cache`); one finding per **bundle**, not per Guava-internal class row. BPA pattern id: `guavaCache`. Not a `code-assessment` pattern — Guava cache usage only occurs in pre-migration code, never native AEMaaCS code. | **Starter prompts (copy-paste):** @@ -226,7 +226,7 @@ The runbook covers **every pattern the migration skill can address**. Each patte | `htlLint` | `html-scan` | heuristic regex scan of `.html` (pure Node — no `rg` binary needed) | | `osgiConfig` | `config-scan` | heuristic scan of OSGi config files for secret-looking keys / `$[secret:]`/`$[env:]` placeholders — **key names + locations only, never secret values** | | `lui`, `cdw`, `templateModernization` | BPA `cascade` → `content-scan` fallback | When a BPA CSV/CAM source is present, these come from BPA (subtypes `custom.classic.widget`; `legacy.dialog.classic`/`.coral2`; `legacy.static.template` + `custom.static.template`). With no BPA source, a heuristic `.content.xml` scan is the fallback — for `templateModernization` it walks `apps//templates/**` at **any depth** (nested/grouped templates included) and classifies each static template as `custom.static.template` or `legacy.static.template` from its page-component resource type, so the custom-vs-legacy distinction survives even without a BPA report. Sample prompts route to **Branch D** (legacy-ui) / **Branch C** (templates), not code-assessment | -| `guavaCache` | `bpa-only` (no analyzer, no content-scan) | BPA is the **sole** source of truth (subtype `com.google.common.cache`), one finding per file. With no BPA source, `guavaCache` has no deterministic fallback and surfaces under **Tier 4 — LLM scan**: the agent greps `.java` files for `import com.google.common.cache` per [references/guava-cache.md](references/guava-cache.md) and tags the result `confidence: llm`. There is deliberately no compiled analyzer detector for this pattern — it does not run inside `code-assessment`'s own discovery. | +| `guavaCache` | `bpa-only` (no analyzer, no content-scan) | BPA is the **sole** source of truth (subtype `custom.guava.cache`), one finding per **bundle** — `identifier` on this subtype is a Guava-internal class, not a customer class, so raw rows are deduped to the bundle named in the message, not surfaced per row. With no BPA source, `guavaCache` has no deterministic fallback and surfaces under **Tier 4 — LLM scan**: the agent greps `.java` files for `import com.google.common.cache` per module, per [references/guava-cache.md](references/guava-cache.md), and tags the result `confidence: llm`. There is deliberately no compiled analyzer detector for this pattern — it does not run inside `code-assessment`'s own discovery. | `htlLint`, `osgiConfig`, and the content-scan **fallback** for `lui`/`cdw`/`templateModernization` are **heuristic** (tagged `confidence: heuristic` in the cache) — candidate matches, not compiler-validated. BPA-sourced `lui`/`cdw`/`templateModernization`/`replication`/`guavaCache` findings are authoritative. Out of scope: `inject-in-sling-model` and `outdated-dependencies` (those belong to code-assessment's own runbook, not migration). diff --git a/plugins/aem/cloud-service/skills/migration/references/guava-cache.md b/plugins/aem/cloud-service/skills/migration/references/guava-cache.md index bfb083dc5..8472e1366 100644 --- a/plugins/aem/cloud-service/skills/migration/references/guava-cache.md +++ b/plugins/aem/cloud-service/skills/migration/references/guava-cache.md @@ -6,13 +6,19 @@ BPA pattern id: **`guavaCache`**. Not a Cloud-Service-native code-quality issue On AEM as a Cloud Service the supported in-process cache library is **Caffeine** (`com.github.benmanes.caffeine.cache.*`). Bundles importing `com.google.common.cache.*` are flagged because Guava is shrinking in the CS uber-jar and relying on Guava's cache from a third-party classloader is unstable. Caffeine is the recommended successor (same author as Guava cache) and its API is intentionally near-identical, so the swap is mechanical with a few well-known call-site renames. -## Discovery — BPA is the source of truth +## Discovery — BPA is the source of truth, at bundle granularity -Findings come from **`getBpaFindings('guavaCache', …)`** (BPA CSV column `subtype` = `com.google.common.cache`). When no BPA/CAM source is available, scan the workspace's `.java` files for `import com.google.common.cache.…` — treat this as a manual, unconfirmed lead per file, not a substitute for BPA. +Findings come from **`getBpaFindings('guavaCache', …)`** (BPA CSV column `subtype` = `custom.guava.cache`, importance `INFO`). -Group by **file**, not by import: a file with multiple `com.google.common.cache.*` imports is one finding, one migration unit — apply the full recipe to that file once. +**BPA's `identifier` on this subtype is a Guava-internal class** (e.g. `com.google.common.cache.AbstractCache`), not a customer class — BPA is bytecode-scanning Guava's own cache implementation wherever it's reachable on a bundle's classpath, not the customer code that imports it. A single bundle that embeds Guava can produce **hundreds of raw CSV rows** (one per Guava-internal class pulled in transitively), all for the same bundle. -BPA gives only a `file` (no `line`/`snippet`) — there is no analyzer detector to resolve those, unlike the `code-assessment` cascade patterns. Open the file directly and locate the `com.google.common.cache.*` imports yourself before editing; do not look for a `guava-cache` entry in the analyzer. +The actionable unit is the **bundle**, named in the free-text `message` field ("The `` class in the `` bundle uses ``."). `getBpaFindings('guavaCache', …)` already dedupes to one target per bundle — do not iterate the raw per-class rows. + +Once you have the bundle name, locate the actual customer files: search that bundle's module (`find /src/main/java -name '*.java' | xargs grep -l 'import com.google.common.cache'`) for the real `import com.google.common.cache.*` occurrences — those are what you edit, not the Guava-internal class named in the BPA row. + +When no BPA/CAM source is available, scan the workspace's `.java` files directly for `import com.google.common.cache.…` per bundle/module — treat this as a manual, unconfirmed lead, not a substitute for BPA. + +BPA gives only a bundle name (no file, no `line`/`snippet`) — there is no analyzer detector to resolve those, unlike the `code-assessment` cascade patterns. Do not look for a `guava-cache` entry in the analyzer; open the module's files directly. ## Classification diff --git a/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js b/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js index 8ae5ade5a..44a6d3e79 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/bpa-local-parser.js @@ -20,7 +20,7 @@ const path = require('path'); const PATTERN_TO_SUBTYPE = { scheduler: "sling.commons.scheduler", assetApi: "unsupported.asset.api", - guavaCache: "com.google.common.cache", + guavaCache: "custom.guava.cache", }; // CSV subtype to pattern mapping (based on actual CSV structure) @@ -29,7 +29,7 @@ const CSV_SUBTYPE_TO_PATTERN = { "javax.jcr.observation.EventListener": "eventListener", "org.apache.sling.api.resource.observation.ResourceChangeListener": "resourceChangeListener", "org.osgi.service.event.EventHandler": "eventHandler", - "com.google.common.cache": "guavaCache" + "custom.guava.cache": "guavaCache" }; // Known scheduler identifier @@ -423,31 +423,50 @@ function processEventHandlerFindings(findings) { } /** - * Process Guava cache findings from CSV. One entry per file/class — a file - * with several `com.google.common.cache.*` imports is still a single - * migration unit, not one finding per import. + * Extract the bundle/module name from a `custom.guava.cache` finding's message. + * + * Unlike scheduler/eventListener/etc., `identifier` on this subtype is a + * *Guava-internal* class (e.g. `com.google.common.cache.AbstractCache`) — + * BPA is bytecode-scanning Guava's own cache implementation classes wherever + * they are reachable, not the customer's classes that import them. The only + * customer-relevant unit is the **bundle** the message names ("The X class + * in the bundle uses Y."). A real bundle can produce hundreds of + * these rows (one per Guava-internal class pulled in) — dedupe to one entry + * per bundle, not per row. + */ +function extractGuavaBundleFromMessage(finding) { + const message = finding.message || ''; + const match = message.match(/\bin the ([\w.-]+) bundle\b/); + return match ? match[1] : null; +} + +/** + * Process Guava cache findings from CSV. One entry per bundle — BPA reports + * every Guava-internal class it finds on that bundle's classpath, not one + * finding per customer file, so grouping by row or by `identifier` would + * bloat to hundreds of entries for a single bundle that just embeds Guava. */ function processGuavaCacheFindings(findings) { const guavaCacheFindings = findings.filter(finding => - finding.subtype === 'com.google.common.cache' + finding.subtype === 'custom.guava.cache' && !String(finding.code || '').startsWith('_') ); const identifiers = {}; - const classNames = []; + const bundleNames = []; guavaCacheFindings.forEach(finding => { - const className = extractClassNameFromCsvFinding(finding); - if (className && !classNames.includes(className)) { - classNames.push(className); + const bundleName = extractGuavaBundleFromMessage(finding); + if (bundleName && !bundleNames.includes(bundleName)) { + bundleNames.push(bundleName); } }); - if (classNames.length > 0) { - identifiers['com.google.common.cache'] = classNames; + if (bundleNames.length > 0) { + identifiers['custom.guava.cache'] = bundleNames; } return { - subtype: 'com.google.common.cache', + subtype: 'custom.guava.cache', identifiers: identifiers }; } @@ -567,13 +586,13 @@ function createUnifiedCollection(bpaData, outputDir) { const mongoSafeSubtype = toMongoSafeFieldName(guavaCacheCollection.subtype); subtypes[mongoSafeSubtype] = {}; - Object.entries(guavaCacheCollection.identifiers).forEach(([identifier, classNames]) => { + Object.entries(guavaCacheCollection.identifiers).forEach(([identifier, bundleNames]) => { const mongoSafeIdentifier = toMongoSafeIdentifier(identifier); - subtypes[mongoSafeSubtype][mongoSafeIdentifier] = classNames; - totalFindings += classNames.length; + subtypes[mongoSafeSubtype][mongoSafeIdentifier] = bundleNames; + totalFindings += bundleNames.length; }); - console.log(`Found ${Object.values(guavaCacheCollection.identifiers).flat().length} guava cache files`); + console.log(`Found ${Object.values(guavaCacheCollection.identifiers).flat().length} bundles using Guava cache`); } // Process content / legacy-UI subtypes (cdw, lui, templates, replication). diff --git a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js index df5a3d104..58d4fff4c 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/runbook-generator.js @@ -25,11 +25,16 @@ * dialogs, custom `cq:Widget` xtypes, static templates) is the * heuristic fallback when no BPA source is available. Routed to * migration Branches D / C, not code-assessment. - * 'bpa-only' — `guavaCache`: BPA/CAM/CSV only (subtype `com.google.common.cache`), - * one finding per file. No analyzer, no content-scan — Guava - * cache usage does not occur in native AEMaaCS code, so there is - * deliberately no compiled detector for it. With no BPA source - * the pattern surfaces in `needsLlmScan` like any other + * 'bpa-only' — `guavaCache`: BPA/CAM/CSV only (subtype `custom.guava.cache`), + * one finding per bundle. `identifier` on this subtype is a + * Guava-internal class, not a customer class — BPA reports + * every Guava-internal class reachable on a bundle's + * classpath, so raw rows are deduped to the bundle named in + * the message, not surfaced per row. No analyzer, no + * content-scan — Guava cache usage does not occur in native + * AEMaaCS code, so there is deliberately no compiled + * detector for it. With no BPA source the pattern surfaces + * in `needsLlmScan` like any other * unscanned pattern. * * `html-scan`/`config-scan`/`content-scan` (fallback) findings are tagged `confidence: 'heuristic'` in the @@ -176,7 +181,7 @@ const PATTERN_META = { severity: 'info', strategy: 'bpa-only', bpaSlugs: ['guavaCache'], - description: 'Bundles importing `com.google.common.cache.*` (Guava in-process cache). Migrate to Caffeine (`com.github.benmanes.caffeine.cache.*`) — a near 1:1 API swap. Not a Cloud-Service-native pattern — only found in code carried over from legacy AEM — so BPA is the sole source of truth; there is no analyzer or content-scan fallback.', + description: 'Bundles importing `com.google.common.cache.*` (Guava in-process cache). Migrate to Caffeine (`com.github.benmanes.caffeine.cache.*`) — a near 1:1 API swap. Not a Cloud-Service-native pattern — only found in code carried over from legacy AEM — so BPA is the sole source of truth; there is no analyzer or content-scan fallback. BPA reports one finding per bundle (identifier is a Guava-internal class, not a customer class).', promptPattern: 'guavaCache', sampleOverride: 'Use the migration skill: fix guavaCache findings using BPA CSV — swap Guava cache for Caffeine.', }, diff --git a/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js b/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js index bf8814300..2473d63df 100644 --- a/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js +++ b/plugins/aem/cloud-service/skills/migration/scripts/unified-collection-reader.js @@ -19,7 +19,7 @@ const PATTERN_TO_SUBTYPE = { eventListener: "javax.jcr.observation.EventListener", resourceChangeListener: "org.apache.sling.api.resource.observation.ResourceChangeListener", eventHandler: "org.osgi.service.event.EventHandler", - guavaCache: "com.google.common.cache" + guavaCache: "custom.guava.cache" }; // MongoDB-safe to pattern mapping @@ -29,7 +29,7 @@ const MONGO_SAFE_TO_PATTERN = { "javax_jcr_observation_EventListener": "eventListener", "org_apache_sling_api_resource_observation_ResourceChangeListener": "resourceChangeListener", "org_osgi_service_event_EventHandler": "eventHandler", - "com_google_common_cache": "guavaCache" + "custom_guava_cache": "guavaCache" }; // Pattern → subtype(s), 1:many. Covers the Java patterns above plus the @@ -46,7 +46,7 @@ const PATTERN_TO_SUBTYPES = { lui: ["legacy.dialog.classic", "legacy.dialog.coral2", "legacy.custom.component", "legacy.static.template"], templateModernization: ["legacy.static.template", "custom.static.template"], replication: ["forward.replication", "reverse.replication"], - guavaCache: ["com.google.common.cache"], + guavaCache: ["custom.guava.cache"], }; // Patterns whose findings are keyed by JCR path (raw keys, generic processor). @@ -327,23 +327,27 @@ function processEventHandlerFromUnified(subtypeData, targets) { /** * Process Guava cache data from unified collection. One target per - * file/class — BPA already reports at file granularity, not per import. + * **bundle** — `identifier` here is a Guava-internal class BPA found on + * that bundle's classpath (e.g. `com.google.common.cache.AbstractCache`), + * not a customer class; the bundle name (in `className`) is the actionable + * unit. A bundle can produce hundreds of raw CSV rows (one per Guava + * internal class reachable) but is still exactly one migration unit. */ function processGuavaCacheFromUnified(subtypeData, targets) { let count = 0; const identifierKeys = Object.keys(subtypeData || {}).sort(); for (const mongoSafeIdentifier of identifierKeys) { - const classNames = subtypeData[mongoSafeIdentifier] || []; + const bundleNames = subtypeData[mongoSafeIdentifier] || []; const identifier = fromMongoSafeFieldName(mongoSafeIdentifier); - for (const className of classNames) { + for (const bundleName of bundleNames) { count++; targets.push(new BpaTarget( "guavaCache", - className, + bundleName, identifier, - `Imports Guava cache: ${identifier}`, + `Bundle uses Guava cache: ${bundleName}`, "info" )); }