The SAP Commerce Code Review Playbook: Process and Checklist

Generic code review advice does not catch the bugs that hurt SAP Commerce projects. A reviewer who knows Java but not the platform will approve a populator full of business logic, a model stored in the HTTP session, an interceptor doing a FlexibleSearch per save, and an ImpEx file that only imports when run by hand in HAC. All four pass compilation, all four pass unit tests, and all four become production incidents.

This playbook has two halves: a review process that scales beyond three developers, and the platform-specific checklist that turns any competent Java reviewer into an SAP Commerce reviewer. It consolidates and updates two CX Works classics for the 2211 and Spring 6 era.

The Process#

Why the ceremony matters#

PCI-DSS and most enterprise compliance regimes require that all code is reviewed before production deployment and that the review is traceable. That makes the pull request not just good practice but audit evidence. The mechanics: review happens before merge to the shared development branch, one feature per pull request, and nothing enters develop without passing review. Spikes and proofs of concept are exempt because their code must be rewritten anyway; do not burn review capacity on throwaway work, and do not let throwaway work skip the process by being quietly promoted to production code.

Branch model#

Feature branch workflow, with branch names carrying the ticket identifier:

git checkout develop && git pull
git checkout -b feature/COM-1234-express-checkout
# work, commit
git pull origin develop        # resolve conflicts on the branch, not in the PR
git push origin feature/COM-1234-express-checkout --set-upstream

Hotfixes branch from the production line instead of develop, and the merge-back direction (hotfix into both master and develop) is exactly the kind of rule that should be written next to the repository, not tribal knowledge. On merge: squash commits, delete the source branch. The branch list should show work in progress, nothing else.

Automate the floor, review the ceiling#

Everything a machine can check is wasted human attention. The CI pipeline that triggers on every pull request should enforce, as hard gates:

• The build compiles (ant clean all on the exact platform version from the manifest)
• The platform starts and initialization succeeds. A broken ant initialize blocks every developer and every test environment; it deserves a pipeline stage of its own
• Zero test failures, no exceptions
• Static analysis passes: no new blocker issues in SonarQube (critical issues too, if your quality bar allows the throughput cost)

With that floor automated, human reviewers spend their time on what machines cannot judge: whether the change solves a real problem, whether the approach is reasonable given the constraints, whether the tests test the right thing, and the platform checklist below.

Two habits that raise review quality more than any tooling:

1. Ask questions instead of making statements. The author has context you do not. "What happens when the cart has 500 entries?" lands better and teaches more than "this will not scale".
2. Review both sides of the stack. Backend developers reviewing only Java and frontend developers reviewing only TypeScript guarantees that the OCC boundary, where most integration bugs live, is reviewed by nobody.

Reject pull requests without tests that validate the change. Not as punishment; because an untested change costs the team more later than the awkward conversation costs now.

The SAP Commerce Checklist#

This is the platform-specific half. Structure your team's own checklist from it, prune what your maturity level does not need yet, and keep it versioned next to the code.

Architecture and extensions#

• Dependency direction is sacred: service layer extensions never depend on facades, facades never depend on web extensions. If extensioninfo.xml says otherwise, the change is wrong regardless of how well it works.
• Extensions are organized by business capability (acmepayment, acmeloyalty), not by technical layer. A dtos extension is an architecture smell.
• Platform and OOTB commerce extensions are never modified directly. Override beans via Spring in your custom extension; keep overridden classes in a mirrored subpackage (de.hybris.platform.commercefacades.order.impl.DefaultCheckoutFacade becomes com.acme.commerce.commercefacades.order.impl.AcmeCheckoutFacade) so the origin is obvious.
• The customize folder mechanism is a last resort with a support ticket attached, and it is not supported on CCv2 builds at all.

Spring wiring#

• Beans are declared in XML configuration, not JavaConfig, and not found via component scan. The exception is @Controller classes in the web application context. This is platform convention: predictable overriding by bean id is what makes the whole customization model work.
• Inject with @Resource (by name), not @Autowired (by type). Half the platform interfaces have multiple implementations, and @Autowired resolves the wrong one or fails outright, including inside ServicelayerTest subclasses.
• Never declare a property in XML and annotate the same field for injection. One mechanism per dependency.
• Spring 6 note for 2211-jdk21: @Required is gone. The old advice to mark mandatory setters with it no longer compiles. Use constructor injection for mandatory dependencies, or assert presence in afterPropertiesSet().
• Data objects used by converters need scope="prototype":

<bean id="userData"
      class="de.hybris.platform.commercefacades.user.data.UserData"
      scope="prototype"/>

A singleton-scoped Data bean is a shared mutable object handed to every request. It works in the demo and corrupts data under load.

• Inject configuration through ${placeholder} or configurationService, never de.hybris.platform.util.Config directly.
• Getting beans from Registry is a smell; when unavoidable, fetch by name and type together: Registry.getApplicationContext().getBean("ehCacheManager", CacheManager.class).

Converters and populators#

• No business logic in populators. They map, nothing else.
• One converter, a list of populators, each populator owning one concern:

<bean id="acmeProductConverter" parent="abstractPopulatingConverter">
  <lookup-method name="createTarget" bean="productData"/>
  <property name="populators">
    <list>
      <ref bean="productBasicPopulator"/>
      <ref bean="productPricePopulator"/>
      <ref bean="acmeAvailabilityPopulator"/>
    </list>
  </property>
</bean>

• Data objects contain primitives, simple types, and other Data objects. Never models. A ProductModel inside ProductData drags the persistence layer into the view and breaks serialization.
• Populate through productFacade.getProductForOptions(product, OPTIONS) consistently so ProductData assembly happens at one cacheable point instead of scattered across tags and controllers.

Models, session, and state#

• Never abuse model attributes for transient state. A later modelService.saveAll() persists your "temporary" flag.
• Nothing large goes into the session, and never models. Store PKs and resolve on read. Check all three session APIs when reviewing: sessionService.setAttribute(), JaloSession.getCurrentSession().setAttribute(), and HttpSession.setAttribute() in controllers.
• Everything in the session must be serializable; session failover on CCv2 serializes to the backing store, and the first non-serializable attribute breaks cart persistence during rolling deployments.
• cartService.hasSessionCart() before getSessionCart(). The getter creates a cart for anonymous users on every call; skipping the check manufactures millions of orphaned carts that your cleanup jobs then have to remove.

Interceptors and dynamic attributes#

• Interceptors guard data integrity, nothing more. Complex business logic belongs in services; an interceptor runs on every save of that type, from every cronjob, import, and sync.
• No expensive operations inside interceptors. A FlexibleSearch in an onValidate turns a 10,000-item ImpEx import into 10,000 extra queries.
• Dynamic attribute handlers must concern only the type they are defined on, and must be cheap: they execute on every read of the attribute, including in Backoffice list views that read it 50 times per page.

FlexibleSearch and DAOs#

• Always parameterized queries. String-concatenated values break the query cache and are an injection risk:

final FlexibleSearchQuery query = new FlexibleSearchQuery(
    "SELECT {pk} FROM {Order} WHERE {user} = ?user AND {date} >= ?from");
query.addQueryParameter("user", user);
query.addQueryParameter("from", startOfDay);   // truncated, cacheable

• Note the truncation: a parameter like new Date() is unique per millisecond, so the result cache never hits. Truncate to the coarsest granularity the business rule allows.
• DAOs return empty collections, never null.
• Iterating a huge relation through the model getter loads everything into memory; for large relations, page through a FlexibleSearch instead.
• Leading-wildcard LIKE '%term%' conditions cannot use an index. On large tables that is a full scan per call; push such lookups to Solr where they belong.

Cronjobs and threading#

• Long-running jobs implement abort: check clearAbortRequestedIfNeeded(cronJob) in the loop and exit cleanly. A job you cannot stop is an operational hostage situation.
• The JobPerformable delegates to a service; logic lives where it can be unit tested.
• Multi-threaded processing inside jobs is where the platform's rules bite hardest. Review for: models shared between threads (they are not thread-safe), threads created without tenant and session initialization, sessions not closed at thread end, exceptions escaping and killing worker threads silently, and ThreadLocal or MDC values not cleaned up.

ImpEx#

• Every ImpEx must be idempotent (INSERT_UPDATE, not INSERT) and must run in its correct phase. The classic failure: a script that depends on a catalog existing, placed in essential data. It works when the developer runs it in HAC, then fails during initialization because at essential-data time no catalog exists. Review placement, not just content:

INSERT_UPDATE syncattributedescriptorconfig; includedinsync; copyByValue; syncJob(code)[path-delimiter=!][unique=true]; attributeDescriptor(qualifier[default='europe1Prices'], enclosingType(ComposedType.code[default='Product']))[unique=true]
; 0 ; 0 ; sync acmeProductCatalog!Staged->Online ;

• Custom ImpEx processors and decorators need justification in the PR description. Most exist because someone did not know the standard syntax feature that already does the job.
• Essential data, project data, and sample data are strictly separated. A system update must never overwrite business-user modifications, and production must never receive sample data. Data patches go through the @SystemSetup mechanism so they are versioned, ordered, and traceable.

Transactions#

• Coarse-grained boundaries, usually at the facade or business-service level.
• No synchronous external calls and no slow operations (email sending is the classic) inside a transaction. Row locks held across a 3-second HTTP call to a tax service serialize your checkout.
• Watch for delayed-store surprises: writing then reading the same data inside one transaction can return stale state.
• AOP-declared transactions with broad pointcuts (*Service.*) hide the boundary from every reader; prefer explicit @Transactional or programmatic Transaction blocks with a finally.

Jalo layer#

ServiceLayer first, always. The reviewable translations:

Logging#

• slf4j, same as the platform. No System.out, no printStackTrace(), no leftover console.log in storefront code.
• Levels with teeth: ERROR means a process terminated abnormally or something unrecoverable happened. WARN means the system worked around a problem. INFO marks major state changes (job start and end, configuration activation, outbound integration calls). DEBUG traces flow decisions. TRACE dumps object state for production debugging where a debugger cannot go.
• No unsafe log statements: a log line that can itself throw (null chain inside the message) masks the original failure.
• Root log level stays at INFO; the platform manages log4j2 configuration, and on CCv2 you adjust logger levels per environment through properties or at runtime in HAC rather than committing DEBUG floods.

Tests#

• @UnitTest or @IntegrationTest at class level so the test suites split correctly in CI; @Test per method; every test asserts something.
• Far more unit tests than integration tests. An integration test that boots the platform to verify a string format is a 90-second tax on every build for no coverage gain.
• Fresh fixtures per test via @Before setup, Mockito with @Mock and @InjectMocks for collaborators:

@UnitTest
@RunWith(MockitoJUnitRunner.class)
public class AcmePriceServiceTest {

  @Mock private PriceRowLookup lookup;
  private AcmePriceService service;

  @Before
  public void setUp() {
    service = new AcmePriceService(lookup);
  }

  @Test
  public void fallsBackToBasePriceWhenNoCustomerPrice() {
    when(lookup.findCustomerPrice(any())).thenReturn(Optional.empty());
    assertThat(service.resolve(product, customer)).isEqualTo(BASE_PRICE);
  }
}

• Review the assertions against the requirement, not against the code. A test that verifies the mock was called proves wiring, not behaviour. And insist on negative paths; the sunny-day-only suite is how "worked in QA" happens.
• Integration tests create their own data and never depend on project or sample data that a catalog change can silently break.

Backoffice framework#

• Never hold org.zkoss.zk.ui.Component references in non-component classes (editors are the repeat offender) unless the author demonstrably understands ZK lifecycle cleanup.
• Renderers are stateless: services injected, no mutable fields.
• Cascading listeners are unmaintainable; flag them.
• Widget classes go in src, not web/src, or cockpit inheritance breaks downstream.

Running the Checklist Without Killing Throughput#

The full list looks heavy. In practice a trained reviewer runs the platform sections relevant to the diff: a populator change triggers the converter and Data-object checks, a new cronjob triggers the threading and abort checks, an -items.xml change triggers the type-system and ImpEx checks. Ten focused minutes per pull request, not an hour of box-ticking.

Two closing rules. First, the checklist is a living document owned by the whole team; when a production incident reveals a missing check, the fix includes a checklist entry. Second, praise in public review comments is not fluff, it is how the codebase's good patterns get identified and copied. A review culture that only points at problems teaches people to hide their code, and hidden code is where the next incident is already growing.