Code Best Practices
The following guidance represents the coding best practices followed by the Elastic Path Commerce product team. We recommend using these best practices as a checklist during code reviews on your extensions.
Java
Prefer streams over loops
Streams are generally more concise and easier to read.
Incorrect:
private Optional<StoreProductSku> getStoreProductSku(final String skuCode) {
for (StoreProductSku storeProductSku : getStoreProductSkus()) {
if (storeProductSku.getSkuCode().equals(skuCode)) {
return Optional.of(storeProductSku);
}
}
return Optional.empty();
}
Correct:
private Optional<StoreProductSku> getStoreProductSku(final String skuCode) {
return getStoreProductSkus().stream()
.filter(storeProductSku -> storeProductSku.getSkuCode().equals(skuCode))
.findAny();
}
Prefer == and != over Objects.isNull and Objects.notNull
Objects.isNull and Objects.notNull are less concise. One exception to this rule is stream predicates.
Incorrect:
public CacheResult<V> get(final K key) {
final Element element = cache.get(key);
if (Objects.isNull(element)) {
return CacheResult.notPresent();
}
return CacheResult.create((V) element.getObjectValue());
}
Correct:
public CacheResult<V> get(final K key) {
final Element element = cache.get(key);
if (element == null) {
return CacheResult.notPresent();
}
return CacheResult.create((V) element.getObjectValue());
}
Keep variable scope as small as possible
Use local variables rather than class fields whenever possible. Pass values as parameters rather than in class fields whenever possible.
Make classes immutable whenever possible and never mutate field objects in singleton classes
If a class is a singleton, methods on that class should never reassign or modify the state of field variables. This can lead to serious race condition bugs that are hard to track down. Immutable classes are always safe from multi-threading race conditions.
All prototype classes should implement equals and hashcode using Apache Commons HashCodeBuilder and EqualsBuilder
Maps, Sets, and other common collection mechanisms will fail in unexpected ways if hashcode and equals are not implemented. We want to be consistent in how these are implemented (by using Apache Commons) to make maintenance easier. Domain classes with a GUID field should implement hashCode and equals only using the GUID field. If you extend AbstractEntityImpl you will get this automatically.
Also, it's extremely important that the equals and hashcode implementations use the same fields.
Correct:
@Override
public boolean equals(final Object other) {
if (this == other) {
return true;
}
if (!(other instanceof AbstractEntityImpl)) {
return false;
}
AbstractEntityImpl otherEntity = (AbstractEntityImpl) other;
return new EqualsBuilder()
.append(getGuid(), otherEntity.getGuid())
.isEquals();
}
@Override
public int hashCode() {
return new HashCodeBuilder()
.append(getGuid())
.toHashCode();
}
All prototype classes should implement toString using Apache Commons ToStringBuilder
This makes debugging and test failures easier to diagnose.
Correct:
@Override
public String toString() {
return new ToStringBuilder(this, ToStringStyle.MULTI_LINE_STYLE)
.append("startDate", getStartDate())
.append("endDate", getEndDate())
.append("defaultQuantity", getDefaultQuantity())
.append("ordering", getOrdering())
.append("associationType", getAssociationType())
.append("targetProduct", getTargetProduct().getUidPk())
.toString();
}
Use timeService.getCurrentTime() instead of new Date()
timeService.getCurrentTime ensures that all services use the database timezone rather than the machine timezone.
Use concurrent collections instead of synchronized collections
Use concurrent collections instead of synchronized collections (unless the collections are expected to be large):
ConcurrentHashMapinstead ofCollections.synchronizedMapCopyOnWriteArrayListinstead ofCollections.synchronizedListCopyOnWriteArraySetinstead ofCollections.synchronizedSet
Concurrent collections are generally more performant. See Difference between Synchronized and Concurrent Collections in Java?.
However, note that this is best suited for situations where "read-only operations vastly outnumber mutative operations". Mutative operations are expensive if the number of items in the collection is large.
When calling Single.error, wrap with Single.defer
Sometimes the exceptions instantiated by Single.error are never thrown, which can cause major performance issues.
Incorrect:
return Single.error(ResourceOperationFailure.notFound(LINE_ITEM_NOT_FOUND));
Correct:
return Single.defer(() -> Single.error(ResourceOperationFailure.notFound(LINE_ITEM_NOT_FOUND)));
When checking method parameters, use Guava’s checkArgument function
If you need to enforce contractual requirements on method parameters (such as "not null"), make use of Guava's checkArgument function.
Incorrect:
if (productSku == null) {
throw new IllegalArgumentException("Product sku parameter cannot be null.");
}
Correct:
Preconditions.checkArgument(productSku != null, "Product sku parameter cannot be null.");
Maven
Define dependency versions in bill-of-materials/pom.xml
It's best to define dependency versions in one place.
Correct:
<properties>
<org.springframework.version>5.3.22</org.springframework.version>
</properties>
<dependencyManagement>
<dependencies>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-core</artifactId>
<version>${org.springframework.version}</version>
</dependency>
</dependencies>
</dependencyManagement>
Define plugin versions in root pom.xml
Plugin versions cannot be defined using Maven dependency management in the bill of materials. Therefore, you should add a version property in the root POM that is used for all plugin version definitions.
Logging
Use SLF4j classes for logging instead of Log4j or Logback classes
Under the hood, Cortex uses Logback while other services use Log4j. SLF4j acts as a consistent bridge layer on top of the underlying logging framework.
Incorrect:
private static final Logger LOG = LogManager.getLogger(CartDirectorImpl.class);
Correct:
private static final Logger LOG = LoggerFactory.getLogger(CartDirectorImpl.class);
Use SLF4j parameters instead of string concatenation when constructing log messages
Using parameterized logging ensures that string concatenation operations are avoided unless the log message actually needs to be written.
Incorrect:
LOG.warn("Unable to find a setting value in the database for path and context: '" + context.getSettingPath() + "', '" + context.getSettingContext() + "'", e);
Correct:
LOG.warn("Unable to find a setting value in the database for path and context: '{}', '{}'", context.getSettingPath(), context.getSettingContext(), e);
When logging exceptions, always pass the exception in the logger "caused by" parameter
Important stack trace information can be lost unless the exception class is passed to the logger.
Incorrect:
LOG.error("Error occurred while processing a batch: " + ex.getMessage());
Correct:
LOG.error("Error occurred while processing a batch", ex);
Database and Liquibase
Use DATETIME field types over TIMESTAMP
TIMESTAMP fields suffer from the year 2038 problem.
Many-to-many tables should not have GUID or UIDPK fields
Many-to-many tables do not need additional identifiers. The join fields themselves are the identifier.
Liquibase scripts should avoid using the sql change type whenever possible
Writing native SQL in the Liquibase sql change type means that your SQL needs to run on all current and future supported database types. If you use the other change types, Liquibase ensures that the SQL that is executed is compatible with all databases automatically.
Incorrect:
<sql>
INSERT INTO TSETTINGDEFINITION
(UIDPK, PATH, VALUE_TYPE, MAX_OVERRIDE_VALUE, DEFAULT_VALUE, DESCRIPTION)
VALUES
(104, 'COMMERCE/STORE/ABANDONEDCARTCLEANUP/maxHistory', 'Integer', '-1', '60', 'This setting controls the number of days of cart history to keep before a Quartz job clears it.')
</sql>
Correct:
<insert tableName="TSETTINGDEFINITION">
<column name="UIDPK" valueNumeric="104" />
<column name="PATH" value="COMMERCE/STORE/ABANDONEDCARTCLEANUP/maxHistory" />
<column name="VALUE_TYPE" value="Integer" />
<column name="MAX_OVERRIDE_VALUES" value="-1" />
<column name="DEFAULT_VALUE" value="60" />
<column name="DESCRIPTION" value="This setting controls the number of days of cart history to keep before a Quartz job clears it." />
</insert>
If a Liquibase CustomChange class is required, implement CustomTaskChange instead of CustomSqlChange
CustomSqlChange classes return an array of SqlStatement objects. This array can get quite large which can lead to memory issues.
For more details, see customChange.
Correct:
public class MyCustomTaskChange implements CustomTaskChange {
@Override
public void execute(final Database database) throws CustomChangeException
}
}
When adding foreign key constraints, explicitly add an index on the foreign key base column
MySQL implicitly adds an index on foreign key “base columns”, but other databases (Oracle, Postgres, etc) do not. Therefore performance may look good on MySQL but will be poor on other databases.
Create the index BEFORE the foreign key, otherwise MySQL will drop the automatically created index and replace it with your index, which might be inefficient. "MySQL requires indexes on foreign keys and referenced keys so that foreign key checks can be fast and not require a table scan. In the referencing table, there must be an index where the foreign key columns are listed as the first columns in the same order. Such an index is created on the referencing table automatically if it does not exist. This index might be silently dropped later if you create another index that can be used to enforce the foreign key constraint."
Correct:
<changeSet id="7.1.0-ordersku-parent-child-lookup" author="elasticpath" context="expand">
<createTable tableName="TORDERSKUPARENT">
<column name="CHILD_UID" type="BIGINT">
<constraints nullable="false"/>
</column>
<column name="PARENT_UID" type="BIGINT">
<constraints nullable="false"/>
</column>
</createTable>
<createIndex tableName="TORDERSKUPARENT" indexName="I_OSKUPARENT_CHILD">
<column name="CHILD_UID"/>
</createIndex>
<createIndex tableName="TORDERSKUPARENT" indexName="I_OSKUPARENT_PARENT">
<column name="PARENT_UID"/>
</createIndex>
<addForeignKeyConstraint constraintName="FK_OSKUPARENT_CHILD"
baseTableName="TORDERSKUPARENT"
baseColumnNames="CHILD_UID"
referencedTableName="TORDERSKU"
referencedColumnNames="UIDPK"/>
<addForeignKeyConstraint constraintName="FK_OSKUPARENT_PARENT"
baseTableName="TORDERSKUPARENT"
baseColumnNames="PARENT_UID"
referencedTableName="TORDERSKU"
referencedColumnNames="UIDPK"/>
</changeSet>
Always specify name when adding unique constraints
When adding unique constraints using Liquibase, the constraintName is not required. However, if we ever need to drop the unique constraint, we will need to specify the constraint name. Therefore we should always be explicit about the constraint name.
Incorrect:
<changeSet id="add-tcoupon-unique-constraint">
<addUniqueConstraint tableName="TCOUPON" columnNames="COUPONCODE_UPPER"/>
</changeSet>
Correct:
<changeSet id="add-tcoupon-unique-constraint">
<addUniqueConstraint constraintName="COUPONCODE_UPPER_UNQ" tableName="TCOUPON" columnNames="COUPONCODE_UPPER"/>
</changeSet>
Liquibase dbms parameter
Avoid adding the dbms parameter to Liquibase changesets. Instead, add it to the Liquibase sql tags within the changeset.
Imagine we create a Postgresql-specific changeset and a MySQL-specific changeset. A project is on MySQL so the MySql-specific changeset is executed. Later, the customer migrates to PostgreSQL. Now Liquibase (incorrectly) tries to execute the Postgresql-specific changeset. Therefore, it’s better to have a single changeset for all databases and have separate sql tags within the changeset with the dbms specification.
Incorrect:
<changeSet id="2022-02-mysql-migrate-some-data" author="elasticpath" context="migrate-data" dbms="mysql">
<comment>Migrate some data.</comment>
<sql>
...MySQL-specific DML...
</sql>
</changeSet>
<changeSet id="2022-02-postgresql-migrate-some-data" author="elasticpath" context="migrate-data" dbms="postgresql">
<comment>Insert all authorizations.</comment>
<sql>
...PostgreSQL-specific DML...
</sql>
</changeSet>
Correct:
<changeSet id="2022-02-migrate-some-data" author="elasticpath" context="migrate-data">
<comment>Migrate some data.</comment>
<sql dbms="mysql">
...MySQL-specific DML...
</sql>
<sql dbms="postgresql">
...PostgreSQL-specific DML...
</sql>
</changeSet>
Avoid changing Liquibase changesets that have been released
When a Liquibase changeset is executed, a checksum value is recorded in the DATABASECHANGELOG table. If the hash code changes for a changeset that was previously executed, Liquibase will throw an exception and the upgrade will fail.
If you absolutely need to change a Liquibase changeset that has been released, you will need to add a validCheckSum field at the top of the changeset definition, as in the example below:
<changeSet id="add-defaultcart-unique-constraint-data-migration"
author="elasticpath" context="migrate-data" dbms="mysql,oracle,postgresql">
<validCheckSum>8:c646ad3bdfe97f12f772d98cb283d749</validCheckSum>
<comment>set default cart to null for inactive and non default carts</comment>
<update tableName="TSHOPPINGCART">
<column name="DEFAULTCART" value="null"/>
<where>STATUS = 'INACTIVE' OR DEFAULTCART = '0'</where>
</update>
</changeSet>
To determine the validCheckSum value, retrieve the value of the MD5SUM field in the DATABASECHANGELOG table from a database that had the original changeset applied before your changes.
Avoid using <validCheckSum>ANY</validCheckSum> as this will prevent detection of future accidental changes to the changeset.
Index and foreign key naming
For consistency, index and foreign key names should follow this pattern:
[I|FK]_[{TABLE_NAME}]_[{FIELD_NAME1}]_[{FIELD_NAME2}]_[{FIELD_NAME3}]_[UNQ]
Index names should start with I_. Foreign key names should start with FK_.
The base table for the index or foreign key should be included ensure global uniqueness, but without the leading T. For example, TBASEAMOUNT becomes BASEAMOUNT, and TPRICELISTASSIGNMENT becomes PRICELISTASSIGNMENT.
The field name(s) should be kept as-is, except without the underscores. To illustrate, CUSTOMER_GUID becomes CUSTOMERGUID, CREATED_DATE becomes CREATEDDATE, LAST_MODIFIED_DATE becomes LASTMODIFIEDDATE, and SHARED_ID becomes SHAREDID. If the index involves multiple fields, they should be separated with an underscore.
The _UNQ suffix should be added if the index has a unique constraint.
Some examples of names following this pattern:
I_PRODUCT_CODE_UNQ: Unique index onTPRODUCT.CODE.I_PRODUCTASSOCIATION_STARTDATE_ENDDATE: Index onTPRODUCTASSOCIATION.START_DATEandTPRODUCTASSOCIATION.END_DATE.FK_PRODUCTASSOCIATION_SOURCEPRODUCTUID: Foreign key constraint onTPRODUCTASSOCIATION.SOURCE_PRODUCT_UID.FK_PRODUCTASSOCIATION_TARGETPRODUCTUID: Foreign key constraint onTPRODUCTASSOCIATION.TARGET_PRODUCT_UID.I_PRODUCTATTRIBUTEVALUE_ATTRIBUTEUID: Index onTPRODUCTATTRIBUTEVALUE.ATTRIBUTE_UID.FK_PRODUCTATTRIBUTEVALUE_ATTRIBUTEUID: Foreign key constraint onTPRODUCTATTRIBUTEVALUE.ATTRIBUTE_UID.
Index and foreign key names can contain up to 63 characters. See Object Name Length Limits for more details.
note
Many existing indexes and foreign keys do not follow this standard, but we will follow this standard for all new indexes and foreign keys going forward.
Referencing related records by UIDPK vs GUID
When adding a field to a table that will reference a record in a separate table (likely with a foreign key constraint), it can be unclear whether to reference the remote table's UIDPK or GUID field.
Our best practice guideline is to use UIDPK if the related record will be loaded eagerly, or GUID if the related record will not be loaded eagerly.
For example, when relating a shopping cart line item to a product sku, we declare the field in AbstractShoppingItemImpl as follows:
@Basic
@Column(name = "SKU_GUID", nullable = false)
public String getSkuGuid() {
return skuGuid;
}
In this case, the records are linked by GUID.
Alternately, when relating a shopping cart to its line items, we declare the field in ShoppingCartMementoImpl as follows:
@OneToMany(targetEntity = ShoppingItemImpl.class, cascade = { CascadeType.ALL }, fetch = FetchType.EAGER)
@ElementJoinColumn(name = "SHOPPING_CART_UID", updatable = false)
@ElementForeignKey(updateAction = ForeignKeyAction.CASCADE)
@ElementDependent
@OrderBy("ordering")
public List<ShoppingItem> getAllItems() {
return allItems;
}
Java Persistence API
JPA classes should not contain relationships outside their domain
Entity graphs should not cross into other domains of the application. For example, transactional entities like shopping cart should not have relationships with catalog entities like product.
DataCache
Transactional entities like shopping cart should always define the @DataCache=false annotation. Also, if your class extends a class with @DataCache=false, your class should too.
Leverage load tuners
The default fetch strategy for OneToMany relationships is lazy and should not be changed unless really necessary. Make use of load tuners to retrieve lazy relationships eagerly when needed. Elastic Path supports two types of load tuners: Fetch group load tuners and programmatic load tuners.
Fetch group load tuners are defined as annotations on JPA entities, as in the following example:
@FetchGroups({
@FetchGroup(name = FetchGroupConstants.CUSTOMER,
attributes = { @FetchAttribute(name = "profileValueMap"),
@FetchAttribute(name = "sharedId") }),
@FetchGroup(name = FetchGroupConstants.ORDER_SEARCH,
attributes = { @FetchAttribute(name = "profileValueMap"),
@FetchAttribute(name = "sharedId") })
})
Programmatic load tuners are classes that can modify which fields are retrieved at runtime, such as ProductLoadTuner.
Best practice is to use programmatic load tuners, but instead of defining them as beans, define them explicitly as needed. This is so that implementations are more likely to define the load tuner with the fields that are really needed, rather than re-using a bean that might not be fit for purpose.
Incorrect:
<bean id="PRODUCT_LOAD_TUNER_ALL" class="com.elasticpath.domain.catalog.impl.ProductLoadTunerImpl" scope="singleton" parent="epDomain">
<property name="loadingSkus" value="true"/>
<property name="loadingProductType" value="true"/>
<property name="loadingAttributeValue" value="true"/>
<property name="loadingCategories" value="true"/>
<property name="loadingDefaultSku" value="true"/>
<property name="loadingLocaleDependentFields" value="true"/>
<property name="productSkuLoadTuner" ref="PRODUCT_SKU_LOAD_TUNER_ALL"/>
<property name="productTypeLoadTuner" ref="PRODUCT_TYPE_LOAD_TUNER_ALL"/>
</bean>
Correct:
final ProductLoadTuner productLoadTuner = BeanLocator.getPrototypeBean(ContextIdNames.PRODUCT_LOAD_TUNER, ProductLoadTuner.class);
final CategoryLoadTuner categoryLoadTuner = BeanLocator.getPrototypeBean(ContextIdNames.CATEGORY_LOAD_TUNER, CategoryLoadTuner.class);
categoryLoadTuner.setLoadingAttributeValue(true);
productLoadTuner.setCategoryLoadTuner(categoryLoadTuner);
productLoadTuner.setLoadingCategories(true);
productLoadTuner.setLoadingAttributeValue(true);
productLoadTuner.setLoadingLocaleDependentFields(true);
Specify an ORDER BY clause for all named queries that return multiple results
If an explicit order by clause is not specified, each database engine will order results differently, which can lead to intermittent failures or inconsistent behavior.
Incorrect:
<named-query name="ORDER_SELECT_ALL">
<query>
SELECT o
FROM OrderImpl as o
</query>
</named-query>
Correct:
<named-query name="ORDER_SELECT_ALL">
<query>
SELECT o
FROM OrderImpl as o
ORDER BY o.orderNumber
</query>
</named-query>
Testing
Prefer Mockito for mocking over JMock
All new unit tests should use Mockito for mocking, rather than JMock. Mockito is a more modern framework with a simpler syntax.
Incorrect:
@Test
public void testGetAppliedRulesByLineItemReturnsEmptyCollection() {
final ShoppingItem shoppingItem = context.mock(ShoppingItem.class);
final ItemDiscountRecordImpl itemDiscountRecord = new ItemDiscountRecordImpl(shoppingItem, RULE_ID, ACTION_ID, BigDecimal.ZERO, 1);
context.checking(new Expectations() {
{
allowing(shoppingItem).getGuid();
will(returnValue(LINE_ITEM_ID));
}
});
promotionRecordContainer.addDiscountRecord(itemDiscountRecord);
Collection<Long> appliedRules = promotionRecordContainer.getAppliedRulesByLineItem(LINE_ITEM_ID);
Assertions.assertThat(appliedRules)
.withFailMessage("There should be no applied rules for the line item")
.isEmpty();
}
Correct:
@Test
public void testGetAppliedRulesByLineItemReturnsEmptyCollection() {
final ShoppingItem shoppingItem = mock(ShoppingItem.class);
final ItemDiscountRecordImpl itemDiscountRecord = new ItemDiscountRecordImpl(shoppingItem, RULE_ID, ACTION_ID, BigDecimal.ZERO, 1);
when(shoppingItem.getGuid()).thenReturn(LINE_ITEM_ID);
promotionRecordContainer.addDiscountRecord(itemDiscountRecord);
Collection<Long> appliedRules = promotionRecordContainer.getAppliedRulesByLineItem(LINE_ITEM_ID);
Assertions.assertThat(appliedRules)
.withFailMessage("There should be no applied rules for the line item")
.isEmpty();
}
note
Also make sure to add the @RunWith(MockitoJUnitRunner.class) annotation to all test classes using Mockito.
Use AssertJ for test assertions instead of JUnit
JUnit is a testing framework that focuses on defining, executing, and reporting tests, while AssertJ is a library that enhances the readability and expressiveness of assertions within those tests.
Incorrect:
Assert.assertThat("A DiscountRecordContainer should be empty when no rules have been applied", promotionRecordContainer.getAppliedRules(), empty());
Assert.assertTrue(brand.getUidPk() != 0);
Correct:
Assertions.assertThat(promotionRecordContainer.getAppliedRules())
.withFailMessage("A DiscountRecordContainer should be empty when no rules have been applied")
.isEmpty();
Assertions.assertThat(brand.getUidPk())
.isEqualTo(0);
Use Assertions.assertThatThrownBy for exception assertions
The expected attribute of the @Test annotation is no longer supported in newer versions of JUnit. Also, the AssertJ assertThatThrownBy method allows much more expressive checks (such as checking the exception message) and is also more specific about which line of the test is expected to throw the exception.
Incorrect:
@Test(expected = EpStructuredErrorMessageException.class)
public void readAccessTokenExpired() {
jwtTokenStrategy.setPublicKeyObjects(ImmutableList.of(expiredTokenKey, validKey));
jwtTokenStrategy.readAccessToken(JWT_TOKEN);
}
Correct:
@Test
public void readAccessTokenExpired() {
jwtTokenStrategy.setPublicKeyObjects(ImmutableList.of(expiredTokenKey, validKey));
Assertions.assertThatThrownBy(() -> jwtTokenStrategy.readAccessToken(JWT_TOKEN))
.isInstanceOf(EpStructuredErrorMessageException.class)
.hasMessageContaining("test");
}
Use the Awaitility library for retry logic
Due to caching, it can sometimes take a few seconds for changes made in Commerce Manager to appear in Cortex. To prevent intermittent test failures, it's sometimes necessary to retry an operation several times until the cache times out to get the expected result.
Using Awaitility allows developers to write simple code that correctly retries with intentional timeouts.
Incorrect:
@Then('^the payment instrument (.+) is not available in profile$')
static void verifyInstrumentNotAvailableInProfile(String instrumentName) {
assertThat(Profile.findInstrument(instrumentName))
int index = 0
int retryCounter = 5
boolean elementExists = Profile.findInstrument(instrumentName)
while (elementExists && index <= retryCounter) {
sleep(SLEEP_THREE_SECONDS_IN_MILLIS)
index++
Profile.findInstrument(instrumentName)
}
assertThat(elementExists)
.as("Payment instrument shouldn't be in profile")
.isFalse()
}
Correct:
@Then('^the payment instrument (.+) is not available in profile$')
static void verifyInstrumentNotAvailableInProfile(String instrumentName) {
Awaitility.await()
.atMost(TEN_SECONDS)
.with().pollDelay(ZERO).pollInterval(ONE_SECOND)
.until { !Profile.findInstrument(instrumentName) }
}
Mocking BeanFactory#getPrototypeBean
When using Mockito to mock BeanFactory#getPrototypeBean, it is important to ensure that each call to getPrototypeBean will return a new instance of the object, rather than returning the same object each time. If done incorrectly, this can lead to confusing failures in tests.
Incorrect:
when(beanFactory.getPrototypeBean(ContextIdNames.PRODUCT_SKU_OPTION_VALUE, ProductSkuOptionValueImpl.class))
.thenReturn(new ProductSkuOptionValueImpl());
Correct:
when(beanFactory.getPrototypeBean(ContextIdNames.PRODUCT_SKU_OPTION_VALUE, ProductSkuOptionValueImpl.class))
.thenAnswer(invocation -> new ProductSkuOptionValueImpl());
Performance
Avoid making db calls from within loops or streams
The ultimate goal for every application is to make as less as possible network roundtrips. Making db calls from loops/streams is highly inefficient.
If you need to retrieve multiple records, create a named query that accepts a list of parameters: i.e. SELECT c FROM CustomerImpl c WHERE c.uidPk in (:list)
Avoid long transactions
All transactions should be as short as possible, so the relevant resources can be optimally shared amongst other threads.
To delete objects with large object graphs, use CASCADE deletion
Instead of programming cascade deletion (via JPA or service methods), the records can be efficiently deleted using database cascade deletion. Provided that all relevant tables are correctly referenced via FKs, when primary record is deleted, all others will be automatically be deleted as well.
Correct:
<addForeignKeyConstraint constraintName="FK_OSHIPMENT_ORDER" onDelete="CASCADE"
baseTableName="TORDERSHIPMENT" referencedTableName="TORDER"
baseColumnNames="ORDER_UID" referencedColumnNames="UIDPK" />
Avoid retrieving an entity when only an update or delete is required
It is not required to fetch the whole entity if a simple field needs to be updated or a record deleted. One JPA SELECT may trigger many needless db calls (roundtrips) and seriously impede the performance.
Correct:
UPDATE ShoppingCartMementoImpl sc SET sc.lastModifiedDate = ?1
WHERE sc.uidPk = ?2
Avoid importing spring contexts using classpath*: unless necessary
The use of wildcards has been known to be inefficient within OSGi/Cortex. The wildcard causes jar files to be scanned repeatedly, significantly slowing application bootup time.
The classpath* syntax is only required if you expect to find multiple files matching the path.
Incorrect:
<import resource="classpath*:spring/sync-tool-context.xml"/>
Correct:
<import resource="classpath:spring/sync-tool-context.xml"/>
Disconnect JPA entity object graphs to improve performance
Often the best way to improve JPA performance is to split large JPA object graphs into smaller entities. There are several benefits to doing this:
- It's usually easier and safer to work with two disconnected entities rather than using load tuners or fetch groups to control the amount of data being retrieved.
- Smaller objects are usually easier to cache and allow more efficient caches.
Deciding where to split the object graph is an important consideration. Good candidates include:
- The boundary between transactional (created by shoppers) and non-transactional (created by business users) objects. For example:
- Attribute value (transactional) --> Attribute (non-transactional)
- Cart line item (transactional) --> Product SKU (non-transactional)
- Many-to-one relationships. When a multiple records link to a single record, this can usually benefit from separate caching of the related object.
When splitting up an object graph, the following changes should be made:
- The database relationship should use a GUID field, not a UID field. This is to ensure that Data Sync will continue to work properly after the objects are disconnected.
- A
@Basicfield likeString relatedObjectGuidshould replace the@ManyToOneor@OneToOnerelationship. - A new service class may be required for the disconnected object type.
- Consider the impact on Data Sync. A new top-level Data Sync object type may be required or merge boundary changes may be required.