> For AI agents: the complete documentation index is available at https://nhanbe.id.vn/llms.txt, the full documentation bundle is available at https://nhanbe.id.vn/llms-full.txt.

# Code Quality, SonarQube and Best Practices

## 28 – Code Quality, SonarQube and Best Practices

> **Target:** 3–4 years experienced AEM Developer **Covers from your additional list:** the Cloud Manager code quality gate in depth, SonarQube and OakPAL rules that matter in practice, and coding standards / review habits. **Project domain:** a global energy technology company's marketing site.

***

## Before we start — the gap between "it works" and "it passes the gate"

Files 26 and 27 both ended by pointing here, and for the same reason: a pipeline that builds successfully and a pipeline that clears the code quality gate are two different achievements. You can write a Sling Model that does exactly what the ticket asked, test it thoroughly, and still watch a Cloud Manager pipeline stop dead at Code Quality because of something the ticket never mentioned — an unclosed `ResourceResolver`, a cognitive-complexity score on a method nobody thought was complicated, or a security hotspot on a line that's actually fine but was never explicitly marked reviewed.

**The honest framing for an interview:** SonarQube and OakPAL aren't obstacles bolted onto AEM development — they're automating a review a senior developer would have done by hand anyway, at a speed and consistency no human review cadence can match. Knowing _what_ they actually check, and _why_ those specific things, is what turns "the pipeline failed" from a mystery into a five-minute fix.

***

## 1. Introduction

### 1.1 Where this sits relative to file 26

File 26 established the shape: Code Quality is the second gate in the pipeline, it runs SonarQube plus OakPAL, and it can block a deployment before anything reaches Stage. This file is what's actually inside that gate — the categories of finding that matter for AEM/Java code specifically, and the habits that keep you from meeting them for the first time in a failed pipeline run.

### 1.2 Two tools, two different questions

- **SonarQube asks:** "Is this code well-written?" — bugs, vulnerabilities, code smells, test coverage, duplication.
- **OakPAL asks:** "Will this package do the right thing to the repository?" — filter correctness, forbidden paths, package structure.

Both show up as "Code Quality gate failed," and confusing which one flagged a problem wastes real debugging time — a theme file 26 already introduced and this file makes concrete with actual rule categories.

### 1.3 A real project example

**Requirement.** A new servlet needed to query products by category for an internal tool.

**What made it a code-quality story, not just a code story.** The first version worked correctly in every manual test. The pipeline blocked it anyway — a **security hotspot** on a line building a JCR-SQL2 query by concatenating a request parameter directly into the query string, and a **code smell** on the servlet's `doGet` method, which had grown to a cognitive complexity score well past the threshold after several conditional branches were added over two sprints.

**The fix.** The query was rewritten to bind the parameter safely rather than concatenate it (file 21's territory, but a Sonar rule catches it independently of whether it happens to be exploitable in this specific case — the rule doesn't know that, and shouldn't have to). The method was split into three smaller ones, each doing one part of the validation.

**The lesson.** Neither finding was wrong to raise, even though neither was actively causing a production bug that day. That's the point of the gate — catching a pattern before it becomes an incident, not after.

***

## 2. Core Concepts

### 2.1 What SonarQube actually measures

Five categories, and knowing the names cold matters because interviewers ask for them directly:

- **Bugs** — code that's likely to behave incorrectly (a null pointer risk, a resource that's opened but not always closed on every path).
- **Vulnerabilities** — a demonstrated security weakness (e.g., a query built by string concatenation from user input).
- **Security Hotspots** — code that _needs a human to look at it_ to decide if it's actually a vulnerability in context — not an automatic fail, but a required, explicit review.
- **Code Smells** — maintainability problems that aren't bugs yet: high cognitive complexity, duplicated logic, an oversized class, a method with too many parameters.
- **Coverage & Duplication** — percentage of new code exercised by tests, and percentage of code that's a near-duplicate of code elsewhere.

### 2.2 "New Code," not "all code" — the single most important concept in this file

Adobe's Cloud Manager quality gate is built around **Clean as You Code**: thresholds apply to **new or changed code in this pull request**, not a retroactive score for the entire codebase. This is why a ten-year-old AEM 6.5-to-Cloud-Service migration with plenty of historical debt can still ship through Cloud Manager cleanly — the gate isn't asking "is this whole codebase perfect," it's asking "did you make it worse."

**Why this matters practically:** it means you cannot blame a blocked pipeline on "legacy code that was already like that." If the gate is failing, the finding is almost always on a line your diff touched. It also means refactoring a large old method you didn't otherwise need to touch can backfire — touching it pulls its pre-existing debt into the "new code" window and can surface old smells as if they were new.

### 2.3 Quality Profile vs. Quality Gate — two different Sonar concepts people conflate

- A **Quality Profile** is the _set of rules_ being applied — Adobe ships an AEM-specific profile with rules tuned for Sling, OSGi, and JCR patterns, on top of the general Java profile.
- A **Quality Gate** is the _pass/fail threshold_ applied to a scan's results — e.g., zero new blocker bugs, all new security hotspots reviewed, coverage on new code above a set percentage.

You can have a rule fire (profile) without it failing the build (gate), if it's below the severity the gate cares about. Interviewers who ask "what's the difference between a profile and a gate" are checking whether you've actually read a Sonar report or just seen the word "SonarQube" in a pipeline log.

### 2.4 The AEM-specific findings that come up again and again

Building on the pattern file 14 already named ("deprecated API use, admin sessions, package structure"), the recurring, AEM-specific findings worth recognising on sight:

- **Unclosed `ResourceResolver`** — acquired via `resourceResolverFactory.getServiceResourceResolver(...)` and never closed on every code path, including exception paths. This is the JCR session leak from file 20, caught statically before it ever exhausts a session pool in production.
- **Use of `getAdministrativeResourceResolver()`** or equivalent admin-session shortcuts — deprecated and flagged, because it bypasses the service-user permission model file 13 covers, and because it doesn't work at all in some Cloud Service contexts.
- **Deprecated Sling/AEM API usage** — a method or class marked `@Deprecated` still compiles and runs, but Sonar flags the call so it doesn't quietly survive into a version where it's removed entirely.
- **`System.out.println` or raw exception printing** instead of a proper SLF4J logger call — a code smell, because it bypasses log level control and doesn't show up in the log aggregation Cloud Manager relies on.
- **High cognitive complexity** on `doGet` / `doPost` methods and Sling Model `@PostConstruct` methods specifically, because those are exactly the methods that accumulate one more `if` every sprint until nobody can trace the logic.
- **Hardcoded configuration values** (a URL, a path, a credential-shaped string) instead of an OSGi `@ObjectClassDefinition` property (file 06) — flagged as both a smell and, for anything credential-shaped, a security hotspot.

### 2.5 What OakPAL adds on top, restated with more rule detail

File 26 introduced OakPAL as "structural package validation." The categories worth naming specifically:

- **Filter validity** — every path a package actually writes to must be covered by a declared filter root; OakPAL can catch content that would install outside the declared scope.
- **Forbidden paths** — writes to paths a project has explicitly disallowed (a common project-level rule: nothing should ever be packaged under `/home`, echoing file 25's data-protection point).
- **Import mode correctness** — flags a `ui.content` filter without `mode="merge"` where the project's own OakPAL configuration requires it, catching file 25's "deployment that deleted a week of content" story _before_ the package is ever installed anywhere.
- **Node type sanity** — content using node types the project doesn't expect, often a sign of copy-pasted content from a different environment or a botched export/import.

### 2.6 Review habits beyond the automated gate

Tooling catches patterns; it doesn't catch judgment. A senior reviewer still asks questions no rule engine asks: _does this Sling Model handle a half-filled multifield the way file 05 warned about? does this new service user actually need write access, or was `jcr:all` granted out of convenience?_ The gate is a floor, not a substitute for a human reviewing the change with the same production-mindedness this whole repository has been building toward.

***

## 3. Internal Working

### 3.1 How the "new code" delta is actually computed

Sonar compares the branch being analysed against a **baseline** — typically the last analysis of the branch it will merge into. Lines that differ from that baseline are "new code"; everything else is "overall code," measured but not gate-enforced. This is why the gate result can change _without you changing your code_ — if the baseline branch moves (someone else merges), your diff's boundary against it can shift too.

### 3.2 Local analysis versus the pipeline's analysis

A `sonar-maven-plugin` (or a standalone `sonar-scanner`) run locally, pointed at the same Sonar server and using the same quality profile, produces the same findings the pipeline would — which is exactly why file 26 recommended running it before opening a pull request. The pipeline's run is authoritative for the gate decision; a local run is a rehearsal, not a substitute.

### 3.3 Marking a finding reviewed versus fixing it

A **bug** or **code smell** is resolved by changing the code — there's no "acknowledge and move on" option that clears the gate. A **security hotspot** is different by design: because it needs human judgment about context, Sonar requires it to be explicitly marked **Reviewed** (safe, with a reason) or **Fixed**, in the tool itself, before the gate treats it as resolved. Silently ignoring a hotspot leaves the gate blocked; there's no bypass at the pipeline level (consistent with file 26 — no gate has a bypass).

### 3.4 Where coverage numbers actually come from

The coverage percentage reported to Sonar comes from a test-coverage tool (typically JaCoCo) run during the Maven build — the same unit tests file 27 covers. Coverage is computed only for **new code**, meaning a change that adds 40 untested lines to an existing well-tested class can still fail the gate, even though the class's overall coverage number looks fine.

***

## 8. Coding Examples — common findings and their fixes

### 8.1 Unclosed ResourceResolver → try-with-resources

```java
// FLAGGED: resolver is never closed if getProducts() throws
public List<Product> loadProducts() throws LoginException {
    ResourceResolver resolver = resolverFactory.getServiceResourceResolver(AUTH_INFO);
    return getProducts(resolver); // if this throws, resolver leaks forever
}

// FIXED: try-with-resources guarantees close() on every exit path,
// including an exception — this is the file 20 leak, closed at the source.
public List<Product> loadProducts() throws LoginException {
    try (ResourceResolver resolver = resolverFactory.getServiceResourceResolver(AUTH_INFO)) {
        return getProducts(resolver);
    }
}
```

### 8.2 Admin session shortcut → service user

```java
// FLAGGED: bypasses the service-user permission model entirely (file 13),
// and doesn't work the same way in every Cloud Service context.
ResourceResolver resolver = resolverFactory.getAdministrativeResourceResolver(null);

// FIXED: a named service user with exactly the permissions this code needs.
Map<String, Object> authInfo = Collections.singletonMap(
    ResourceResolverFactory.SUBSERVICE, "product-catalog-service");
ResourceResolver resolver = resolverFactory.getServiceResourceResolver(authInfo);
```

### 8.3 High cognitive complexity → extracted methods

```java
// FLAGGED: one method doing validation, lookup, and formatting,
// with nested conditionals that accumulated over several sprints.
public String renderPrice(Resource resource) {
    if (resource != null) {
        ValueMap vm = resource.getValueMap();
        if (vm.get("price", Double.class) != null) {
            double price = vm.get("price", Double.class);
            if (price > 0) {
                if (vm.get("currency", String.class) != null) {
                    // ... more nesting to format currency-specific output
                }
            }
        }
    }
    return "";
}

// FIXED: each responsibility split into its own small, testable method.
// Cognitive complexity drops because each method reads as one clear idea,
// and each one can be unit tested in isolation (file 27's territory).
public String renderPrice(Resource resource) {
    if (resource == null) return "";
    Double price = resource.getValueMap().get("price", Double.class);
    if (price == null || price <= 0) return "";
    String currency = resource.getValueMap().get("currency", String.class);
    return formatCurrency(price, currency);
}

private String formatCurrency(double price, String currency) {
    // ... formatting logic alone, easy to reason about and test
    return String.format("%.2f %s", price, currency != null ? currency : "USD");
}
```

### 8.4 Hardcoded configuration → OSGi property

```java
// FLAGGED as both a smell and a hotspot if it looks credential-shaped.
private static final String API_ENDPOINT = "https://internal-api.example.com/v2/products";

// FIXED: an OSGi configuration property (file 06), so the value
// is environment-specific and never hardcoded into compiled code.
@ObjectClassDefinition(name = "Product Catalog Service Config")
@interface Config {
    @AttributeDefinition(name = "API Endpoint")
    String apiEndpoint() default "";
}
```

***

## 9. Common Mistakes

| Mistake                                                                        | Why it happens                                                | The actual cost                                                                   |
| ------------------------------------------------------------------------------ | ------------------------------------------------------------- | --------------------------------------------------------------------------------- |
| Treating a security hotspot as a false positive by default                     | It doesn't fail the build automatically, so it feels optional | An actual vulnerability shipped because nobody genuinely reviewed the flag        |
| Refactoring an old, untouched method inside a feature pull request             | Seemed convenient while already in the file                   | Unrelated pre-existing debt surfaces as "new code" and fails the gate             |
| Assuming 100% coverage on a new class guarantees the coverage gate passes      | Doesn't account for shared-diff lines from other files        | A coverage failure that looks inexplicable until the diff is checked line by line |
| Confusing an OakPAL failure for a SonarQube failure                            | Both appear under "Code Quality"                              | Time spent reviewing code style when the real issue is a package filter           |
| Relying only on the pipeline to catch findings                                 | No local Sonar setup                                          | Every fix costs a full pipeline run instead of a local one                        |
| Copy-pasting an existing filter entry without checking its `mode` attribute    | Assumed it was already correct                                | Repeats file 25's content-deletion pattern, sometimes caught only by luck         |
| Using `getAdministrativeResourceResolver()` for convenience during a quick fix | Faster to write than setting up a service user                | Flagged, and in some Cloud Service contexts doesn't behave as expected anyway     |
| Writing one large method instead of several small ones under deadline pressure | Feels faster in the moment                                    | Cognitive complexity findings later, and a method nobody wants to touch           |

***

## 10. Best Practices

- **Run Sonar locally before opening a pull request**, using the same profile the pipeline enforces — catching a finding on your own machine costs minutes; catching it in the pipeline costs a full run.
- **Treat every security hotspot as requiring an actual decision**, documented with a reason when marked reviewed — never a reflexive dismissal.
- **Keep refactoring separate from feature work** where possible, so old debt surfacing as "new code" doesn't block an unrelated feature.
- **Always close a `ResourceResolver` (or JCR `Session`) with try-with-resources**, on every code path, without exception.
- **Never use an admin-session shortcut** — use a named service user scoped to exactly what the code needs (file 13).
- **Push configuration, not hardcoded values, into OSGi properties** — anything environment-specific or credential-shaped belongs there, not in compiled code.
- **Write OakPAL rules in response to real incidents**, the way the `mode="merge"` check in Story 3 was — a quality gate configuration should get stricter exactly where the project has actually been burned before.
- **Don't treat a green pipeline as equivalent to "reviewed."** The gate catches mechanical patterns; a human still needs to judge architectural fit and business-logic correctness.

***

## 11. Debugging Tips

| Symptom                                                 | Where to look                                                                    | What it usually means                                                         |
| ------------------------------------------------------- | -------------------------------------------------------------------------------- | ----------------------------------------------------------------------------- |
| "Code Quality gate failed" with no obvious code problem | The OakPAL section of the report, not just Sonar                                 | A package filter/structure issue, not a code style issue                      |
| Coverage failure on a class that looks well-tested      | The diff's exact line range, not just the class overall                          | Uncovered lines from another part of the same file got pulled into "new code" |
| A finding on a line you didn't write                    | Whether the file was touched by an unrelated formatting or refactor change       | The "Clean as You Code" new-code window pulling in old debt                   |
| A security hotspot that seems clearly safe              | Whether it's actually been marked Reviewed in Sonar, not just judged safe by eye | The gate only clears once the tool itself records the review                  |
| Repeated findings of the same type across a team        | The team's shared coding habits, not individual mistakes                         | Worth a five-minute team conversation rather than five individual fixes       |

***

## 12. Performance Notes

- Running Sonar analysis locally adds build time, but it's minutes against the alternative of a full pipeline round trip that can take much longer end to end (file 26).
- A large, uninterrupted refactor pull request analyses slower and risks pulling more old debt into the new-code window than several smaller, focused ones.
- Coverage instrumentation (JaCoCo) adds modest overhead to the test run itself — negligible next to the cost of discovering a coverage gap only in the pipeline.

***

## 15. Comparison Tables

|                                | **Bug**                           | **Vulnerability**              | **Security Hotspot**             | **Code Smell**                      |
| ------------------------------ | --------------------------------- | ------------------------------ | -------------------------------- | ----------------------------------- |
| What it means                  | Likely-incorrect behaviour        | Demonstrated security weakness | Needs human judgment to classify | Maintainability risk, not incorrect |
| Blocks the gate automatically? | Yes, at blocker/critical severity | Yes                            | Only if left unreviewed          | Yes, at higher severities           |
| Fixed by                       | Changing the code                 | Changing the code              | Marking Reviewed or Fixed        | Changing the code                   |

|                          | **Quality Profile**                 | **Quality Gate**                           |
| ------------------------ | ----------------------------------- | ------------------------------------------ |
| What it is               | The rule set applied                | The pass/fail threshold                    |
| Who sets it              | Adobe (AEM-specific + general Java) | Adobe defaults, configurable on some tiers |
| Can it vary per project? | Rarely                              | Sometimes                                  |

|              | **SonarQube**                            | **OakPAL**                                 |
| ------------ | ---------------------------------------- | ------------------------------------------ |
| Checks       | Code quality                             | Package structure                          |
| Runs against | Source and compiled code                 | Built content packages                     |
| Catches      | Leaks, complexity, hotspots, duplication | Filter errors, forbidden paths, node types |

|                     | **New Code**                                    | **Overall Code**            |
| ------------------- | ----------------------------------------------- | --------------------------- |
| Gate-enforced?      | Yes                                             | No (measured, not enforced) |
| Comparison baseline | The branch being merged into                    | N/A                         |
| Why it exists       | Lets legacy debt ship without blocking new work | Historical visibility only  |

***

## Next topic

**Security best practices for AEM developers** — the security-specific patterns and mistakes this file touched on structurally (unclosed sessions, admin shortcuts, query injection) covered in depth, alongside AEM-specific concerns like XSS through HTL, CSRF protection, and dispatcher-level security already introduced in file 19.

***

_Topic 28 of the AEM Interview Preparation repository. Teaching style, energy-sector project domain._