The XWiki project is following a specific coding style for Java code. We're using Checkstyle (checkstyle.xml) to ensure compliance of the code. Our build (the Maven one) is configured to fail on violations. This is part of the automated checks, see there for ways to skip if necessary at times. However the decision to follow this code style and enforce it was only made long after the beginning of the project and not all the code base has been moved to this new code style. Hence:

• We're only enforcing the code style in the code that has been moved to the new code style. The checked files are defined in xwiki/core/pom.xml (bottom of file).
• We're asking new code to follow the new style and then once a Java file is compliant, to edit xwiki/core/pom.xml and add it there so that we cannot regress...

For examples of "clean" class see the following example and its unit tests:

• DefaultObservationManager.java (main sources|tests)

Configuring your IDE to use the XWiki code style

Eclipse

Download codestyle-eclipse-java.xml.

After this, select Window > Preferences, and open up the configuration for Java > Code Style > Code Formatter. Click on the button labeled Import... and select the file you downloaded. Give the style a name, and click OK.

To reformat a file, press Ctrl+Shift+F while inside that file. To format only a portion of the file, select it and press the same key combination.

Download codetemplates-eclipse.xml.

After this, select Window > Preferences, and open up the configuration for Java > Code Style > Code Templates. Click on the button labeled Import... and select the file you downloaded. You can enable "Automatically add comments for new methods and types" if you want.

To generate a javadoc, press Meta+Shift+J while on the element you want to document.

IntelliJ IDEA

• Set up IntelliJ IDEA code styles:
• Download codestyle-idea.xml.
• Go to IntelliJ's File > Settings and to Editor > Code Style and select Import Scheme > IntelliJ IDEA Code Style XML as shown on

  

• Set up File Templates (used when creating a new Java class, Interface, etc):

  Download idea-fileTemplates-xwiki.tar.gz.

  Close IntelliJ IDEA. Ungzip and untar the file in the configuration directory (or copy unzipped "fileTemplates" directory in the following location):

  • For Mac: in ~/Library/Application Support/JetBrains/<IDEA VERSION>
  • For Linux: in case if installed from JetBrains website in ~/.config/JetBrains/<IDEA VERSION> or if installed from Linux Software app in ~/.var/app/com.jetbrains.IntelliJ-IDEA-Community/config/JetBrains/<IDEA VERSION>
  • For Windows: in C:\Users\<username>\AppData\Roaming\JetBrains\<IDEA VERSION>
• If codestyle is not imported automatically, go to Other Settings > Default Settings > Java codestyle > Set from (XML) > select file downloaded above.
• Restart Intellij IDEA.

Interface best practices

Do not use 'public' in interfaces

Public is always implied in interfaces. Do not write:

public interface Page
{
    public String getParentSpaceKey();
}

But instead, write

public interface Page
{
    String getParentSpaceKey();
}

Make sure your code is backward compatible

Adding a new method to a public interface is a breaking change and your build will fail with error:

But it's possible to make a new interface method backward compatible by providing a default implementation (using the default keyword as in):

@Unstable
default MethodType myNewMethod()
{
  return myOtherMethod();
}

Javadoc Best Practices

We are following the Oracle Javadoc coding conventions. Please make sure you're familiar with them when you write javadoc.

Write useful comments

Do not repeat the name of the method or any useless information. For example, if you have:

/**
 * @return the id
 */
public String getId()
{
    return this.id;
}

Instead, write:

/**
 * @return the attachment id (the id is the filename of the XWikiAttachment object used to construct this Attachment object)
 */
public String getId()
{
    return this.id;
}

In general, useful comments also explain the WHY (e.g. why the method/class exists, when should it be used), on top of the WHAT (what does it do) which is often obvious. Explain the edge cases, error conditions, and avoid spending too much time stating the obvious.

Useful javadoc also provide example of usages (and for method parameters, examples of valid values to pass).

Do not duplicate Javadoc

If you inherit from an interface/class then you shouldn't copy the Javadoc from the super type. Instead you should reference it or add more fine-tuned explanations. For example if getSomething() is the implementation of a method defined in an inherited Something interface or parent class, you shouldn't write:

/**
 * Do something blah blah.
 */
public void doSomething()
{

[...]

Instead, write either the following:

/**
 * {@inheritDoc}
 *
 * <p>
 * Optionally add here javadoc additional to the one inherited from the parent javadoc.
 */
@Override
public void doSomething()
{
[...]

or (it you don't have anything else to add in the javadoc):

@Override
public void doSomething()
{
[...]

Use {@return} where appropriate

The general idea is to not duplicate summary comments with parameters comments.

For simple enough methods returning a value, use the {@return description} combo tag, which generates both the method summary and the @return documentation from a single sentence (Javadoc turns it into "Returns description." and reuses it as the @return text).

Instead of writing:

/**
 * Returns the attachment id (the id is the filename of the XWikiAttachment object used to
 * construct this Attachment object).
 *
 * @return the attachment id
 */
public String getId()
{
    return this.id;
}

Write:

/**
 * {@return the attachment id (the id is the filename of the XWikiAttachment object used to
 * construct this Attachment object)}
 */
public String getId()
{
    return this.id;
}

Rules:

• Use it only for simple getter-like methods: a method returning a value whose entire behaviour fits in the one or two sentences of the tag. It is not a way to skip writing a proper summary: for anything non-trivial, write a dedicated method description followed by a dedicated, regular @return tag (see the Oracle How to Write Doc Comments guidelines on writing good summaries).
• Only for methods that return a value: never on void methods or constructors.
• No leading "Returns"/"Gets" verb (Javadoc adds "Returns") and no trailing period inside the braces: the tag content is a sentence fragment, e.g. {@return the attachment id}.
• When overriding and you only need to point at the inherited @return text, you may combine it with {@inheritDoc}:

  /**
   * {@return {@inheritDoc}}
   *
   * <p>
   * Add other details here...
   */
  @Override
  public String getId()
  {
      ...
  }

Use version and since javadoc tags

For example:

/**
 * Something, blah blah...
 *
 * @version $Id$ 
 * @since 16.0.0RC1
 */

Use one @since per version

When introducing a class or a new method on several branches, multiple @since annotations must be used to mention the different versions in which this element has been added.

For example:

[...]
 * @since 7.4.5
 * @since 8.2.2
 * @since 16.0.0RC1
 */

(and not @since 7.4.5, 8.2.2, 16.0.0RC1)

Use the right format for examples

If you need to use example and have what you past be not modified, use the {@code}} construct, as in:

 * <pre>{@code
 * <plugin>
 *   <groupId>org.xwiki.platform</groupId>
 *   <artifactId>xwiki-platform-tool-provision-plugin</artifactId>
 *   <version>...version...</version>
 *   <configuration>
 *     <username>Admin</username>
 *     <password>admin</password>
 *     <extensionIds>
 *       <extensionId>
 *         <id>org.xwiki.contrib.markdown:syntax-markdown-markdown12</id>
 *         <version>8.5.1</version>
 *       </extensionId>
 *     </extensionIds>
 *   </configuration>
 *   <executions>
 *     <execution>
 *       <id>install</id>
 *       <goals>
 *         <goal>install</goal>
 *       </goals>
 *     </execution>
 *   </executions>
 * </plugin>
 * }</pre>

* <pre><code>
 * &#60;plugin&#62;
 *   &#60;groupId&#62;org.xwiki.platform&#60;/groupId&#62;
 *   &#60;artifactId&#62;xwiki-platform-tool-provision-plugin&#60;/artifactId&#62;
 *   &#60;version&#62;...version...&#60;/version&#62;
 *   &#60;configuration&#62;
 *     &#60;username&#62;Admin&#60;/username&#62;
 *     &#60;password&#62;admin&#60;/password&#62;
 *     &#60;extensionIds&#62;
 *       &#60;extensionId&#62;
 *         &#60;id&#62;org.xwiki.contrib.markdown:syntax-markdown-markdown12&#60;/id&#62;
 *         &#60;version&#62;8.5.1&#60;/version&#62;
 *       &#60;/extensionId&#62;
 *     &#60;/extensionIds&#62;
 *   &#60;/configuration&#62;
 *   &#60;executions&#62;
 *     &#60;execution&#62;
 *       &#60;id&#62;install&#60;/id&#62;
 *       &#60;goals&#62;
 *         &#60;goal&#62;install&#60;/goal&#62;
 *       &#60;/goals&#62;
 *     &#60;/execution&#62;
 *   &#60;/executions&#62;
 * &#60;/plugin&#62;
 * </code></pre>

Trailing Whitespace

Trailing whitespace is prohibited except for one case.
In empty lines in a javadoc comment, a single trailing space character is acceptable but not required.

/**
 * The Constructor.
 * 
 * $param something...
 */

The trailing whitespace in the center line in that comment is permissible. See this proposal for more information.

Class/Interface names

• Prefix class names with Abstract for abstract classes
• Class names should start with an uppercase letter
• The interface name should be as short and expressive as possible with no technical prefix or suffix. For example "Parser".
  • As a consequence interfaces shouldn't be prefixed with "I" (as in "IParser") or suffixed with "Interface" (as in "ParserInterface"), nor suffixed with "IF" (as in "ParserIF).
• Classes implementing interfaces should extend the interface name by prefixing it with a characteristic of the implementation. For example "XWikiParser".
  • As a consequence implementation classes shouldn't be suffixed with "Impl", "Implementation", etc.
• Default implementation classes where there's only one implementation provided by XWiki should be prefixed with "Default". As in "DefaultParser".

Members and fields names

• All methods and fields names should be camelCase, starting with a lower letter (someProperty, getSomeProperty())
• The names should be understandable and short, avoiding abbreviations (parentDocument instead of pdoc, for example)
• Constants should all be uppercase, with underscores as word separators, and no prefix letter (WIKI_PAGE_CREATOR instead of WIKIPAGECREATOR)
• Constants should be public/private static final in classes (public static final String CONTEXT_KEY = "theKey") and without any modifiers in interfaces, since public, static and final are implied and enforced (String PREFERENCES_DOCUMENT_NAME = "XWiki.XWikiPreferences")

Package names

• All code that is not located in the oldcore module should use org.xwiki.
• The package name for code using the component-based architecture must be of the format org.xwiki.(module name).*. For example org.xwiki.rendering.
• Non user-public code must be located in an internal package just after the module name. For example: org.xwiki.rendering.internal.parser.. General rule is org.xwiki.(module name).internal.
• Script Services component implementations should be located in a script package and in a non-internal package. This is because they are considered API and we wish to have our backward-compatibility tool report any breakage (and also so that they are included in the generated Javadoc). If you still need to expose a script service in an internal package the class name should end with InternalScriptService to not fail the build rule.

Logging Best Practices

Getting a Logger

• Use SLF4J. Specifically, if your code is in a Component it must get a Logger using the following construct:

  import org.slf4j.Logger;
  ...
  @Inject
  private Logger logger;

  If not inside a Component, a Logger can be retrieved through:

  import org.slf4j.Logger;
  ...
  private static final Logger LOGGER = LoggerFactory.getLogger(MyClass.class);

Choosing the Level

Use the level according to the following rules:

For example:

// info: what the admin needs to see when the instance starts
this.logger.info("Using permanent directory [{}]", this.permanentDirectory);

// debug: useful when diagnosing a problem, hidden by default
this.logger.debug("Extension [{}] already installed on namespace [{}]", extensionId.getId(), namespace);

// warn: the instance keeps working, the root cause message is enough
this.logger.warn("Failed to resolve the entity [{}]. Cause: [{}]", entityReference, getRootCauseMessage(e));

// error: a developer will need to debug this, so the stack trace is required
this.logger.error("Failed to get document [{}] from the database.", document.getDocumentReference(), e);

Rules:

• Do not print a stack trace when you output a warning. Stack traces fill the logs and should be reserved for errors: in the view of users a stack trace is synonymous with an error, and we want it to be easy for admins to visually check the log files and see the important problems. In order to display the root cause without displaying a full stack trace, use org.apache.commons.lang3.exception.ExceptionUtils:

  // Do NOT write this: getMessage() on the caught exception hides the root cause
  this.logger.warn("Failed to determine if the index exists: [{}]. Trying to recreate the index..", e.getMessage());
  
  // Write this instead
  this.logger.warn("Failed to determine if the index exists: [{}]. Trying to recreate the index..",
      ExceptionUtils.getRootCauseMessage(e));

• Always pass the exception as the last argument of an error() call, never as a message parameter, so that SLF4J prints its stack trace:

  // Do NOT write this: the stack trace is lost
  this.logger.error("Failed to get document [{}]: [{}]", reference, ExceptionUtils.getRootCauseMessage(e));
  
  // Write this instead
  this.logger.error("Failed to get document [{}]", reference, e);

Writing the Message

Rules:

• Never concatenate values into the message. Use the {} placeholders and the matching SLF4J signature to not incur performance penalty:

  // Do NOT write this: the String is built even when the debug level is disabled
  this.logger.debug("Test message with [" + param1 + "] and [" + param2 + "]");
  
  // Write this instead
  this.logger.debug("Test message with [{}] and [{}]", param1, param2);

• Always log as much information as possible to make it easier to understand what's going on:

  // Do NOT write this: nothing tells which document failed
  this.logger.error("Failed to save the document", e);
  
  // Write this instead
  this.logger.error("Failed to save document [{}] with version [{}]", document.getDocumentReference(), version, e);

• Surround parameters with [] in order to separate visually the text from the parameters, and also to clearly notice when leading/trailing spaces are located in parameters:

  this.logger.debug("Extension [{}] already installed on namespace [{}]", extensionId.getId(), namespace);

• Do not add the [] when the toString() of the parameter already wraps the value inside brackets, otherwise you get double brackets. This is the case for example for Collection/List/Set/Map/array, but it could also be the case for any Object that has a toString() generating brackets around the value:

  // Do NOT write this: it displays "Found the following JARs: [[url1, url2]]"
  this.logger.debug("Found the following JARs: [{}]", jars);
  
  // Write this instead: it displays "Found the following JARs: [url1, url2]"
  this.logger.debug("Found the following JARs: {}", jars);

  Note that toString() is called on the passed parameter when the message is formatted for display, which is only one of the things XWiki does with a parameter, see the next section.

Choosing What to Pass as a Parameter

A logging parameter is passed as an Object and XWiki keeps it as one, because log appenders are not the only consumers of a log event.

Any code can end up running inside a Job without knowing anything about jobs: AbstractJobStatus pushes a log listener, so every logging call that is enabled at its level on that thread is captured whatever the class that emitted it. Since warn and error are always enabled with the default logging configuration, those are always captured. The resulting LogEvent keeps the parameters as Objects, nothing is formatted at log time, and the job log is then XStream-serialized to disk (XStreamFileLoggerTail, argument by argument in SafeMessageConverter) to be read back later when the log is displayed.

Consumers then read the parameters back by type, and not only to render them: the log displayers show some types richly, for example an Entity Reference or an Extension Id as a link, and Importer casts log.getArgumentArray()[0] straight to EntityReference. Converting such a parameter to a String does not merely lose the formatting, it breaks the consumer.

Two more mechanics decide the rules below. On write, a parameter is kept as a full object graph unless its class is a Component, or is annotated @Serializable(false), or is one of a few well-known classes (Logger, Provider, ComponentManager, InputStream, OutputStream) — everything else, including anything that merely happens to implement java.io.Serializable, is walked field by field. On read, any failure becomes null, so a parameter whose class can no longer be resolved is lost, where a String would have survived.

Thus, for each parameter, choose between passing the object itself and converting it to a String in the logging call.

Pass the object

Pass the object when it is a small, stable value type whose class is always resolvable and that a consumer can render or use richly:

• an EntityReference / DocumentReference or any other model reference
• an ExtensionId, a Version
• an enum, a String, a Number, a Boolean, a File
• a Collection, whose toString() already brackets its content, so do not add [] around it in the message either
• a Component instance — the converter already replaces it by its toString(), so passing the plain object is equivalent

this.logger.warn("Failed to resolve the entity [{}]. Cause: [{}]", entityReference, getRootCauseMessage(e));
this.logger.error("Failed to parse extension [{}]", installedExtension.getId(), e);

Convert it to a String in the logging call

Cases:

• A document. XWikiDocument and Document are the most frequent mistake: their toString() only returns the full name, but the objects themselves hold the content, the attachments and the document history, and a Document also holds the XWikiContext and therefore a reference to the whole wiki. Logging one inside a Job dumps all of that into the job log. Pass the reference instead:

  // Do NOT write this: the whole document, its history and (for Document) the whole wiki are serialized
  this.logger.error("Failed to save document [{}]", document, e);
  
  // Write this instead
  this.logger.info("Computing differences for document [{}]", document.getDocumentReferenceWithLocale());

• An arbitrary Object whose type is not known at compile time, an event source or a progress step source for instance: nothing useful can be kept and its object graph is unbounded:

  // Build the String on purpose (see SafeMessageConverter): log arguments are kept as
  // objects in the captured LogEvent and XStream-serialized into the job log, where an
  // arbitrary source would bloat the log file and be read back as null.
  LOGGER.warn("Could not find any matching step for source [{}]. Ignoring EndStepProgress.",
      String.valueOf(source));

• A live resource such as a Hibernate Session, a connection or a stream. Several of those implement Serializable, so they would be serialized field by field:

  // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
  // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a Session is
  // a live resource that implements Serializable, so it would be walked field by field.
  LOGGER.warn("Cleanup of session was needed: [{}]", session.toString());

• A mutable builder. A StringBuilder is serialized as its internal character array, and its content can still change between the logging call and the display:

  // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
  // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a StringBuilder
  // would be written out as its internal char array.
  this.logger.debug("Find mail statuses for query [{}] and parameters [{}]", queryString,
      builder.toString());

• A request or any other large object graph. A Request is Serializable, so a Job logging its own request would store a copy of it inside its own log:

  // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
  // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a RemoteEventData
  // is Serializable by design since it is the replicated payload, so it would be written out in full.
  this.logger.debug("Send JGroups remote event [{}]", remoteEvent.toString());

• An object whose toString() is deliberately narrower than its fields. For example a mail configuration whose toString() masks the SMTP password that the object itself carries in clear text, or a mail whose toString() omits the body and the attachments:

  // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
  // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a Mail holds
  // the body and the attachments, none of which its toString() prints.
  LOGGER.info("Sending email [{}]", mail.toString());

• A Class or a Type coming from an extension JAR, which may no longer be resolvable when the log is read back and would then be read back as null. Use getName() or getTypeName() rather than toString():

  logger.error("Unexpected exception when accessing property [{}]", propertyClass.getName(), e);

Rules When Converting to a String

Rules:

• State the reason in a comment next to the logging call, so that neither the next reader nor the next automated pass removes the conversion. All the examples above do this.
• Use String.valueOf(parameter) rather than parameter.toString() when the parameter can be null, so that null is displayed instead of a NullPointerException being thrown:

  // Do NOT write this: source can be null here
  LOGGER.warn("Could not find any matching step for source [{}]. Ignoring EndStepProgress.", source.toString());
  
  // Write this instead
  LOGGER.warn("Could not find any matching step for source [{}]. Ignoring EndStepProgress.",
      String.valueOf(source));

• Do not add a level guard around a warn() or an error() call: those levels are always enabled with the default logging configuration, so converting a parameter to a String there costs nothing. Only debug() and trace() calls benefit from a guard:

  private void logQuery(String queryString, Map<String, Object> filterMap)
  {
      if (this.logger.isDebugEnabled()) {
          StringBuilder builder = new StringBuilder();
          ...
          // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
          // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a StringBuilder
          // would be written out as its internal char array.
          this.logger.debug("Find mail statuses for query [{}] and parameters [{}]", queryString,
              builder.toString());
      }
  }

Never Silently Remove an Explicit Conversion

An existing conversion looks removable for three reasons that all sound right and are all wrong. Before deleting one, check it against these:

• "SLF4J calls toString() itself, so this is redundant." SLF4J is only one of the consumers, see above.
• "It is eager, so it costs performance." warn and error are always enabled, so nothing is saved by deferring. It only matters under a debug/trace level guard.
• "It throws a NullPointerException on null." True, and the fix is String.valueOf(parameter), not passing the object.

Handling SonarQube's java:S2629 Rule

SonarQube's java:S2629 rule ("Preconditions and logging arguments should not require evaluation") fires on the eager String, so keep the conversion and silence the rule where it is deliberate. Per its implementation it:

• only examines parameters whose static type is String, so passing the raw object is never reported;
• exempts no-argument get*()/is*() calls, so getName() and getTypeName() produce no issue, while toString(), String.valueOf(parameter) and getRootCauseMessage(e) (a get*() with a parameter) all do;
• skips the logging call entirely when it is inside a catch block or inside a level guard such as if (this.logger.isDebugEnabled()), which is where most of the conversions above happen to sit;
• does not follow local variables, so assigning to a String local first also produces no issue.

Thus:

• Prefer getName() or getTypeName() to toString() when both are possible, since no issue is then reported:

  // Reported by java:S2629
  logger.error("Unexpected exception when accessing property [{}]", propertyClass.toString(), e);
  
  // Not reported, and just as readable
  logger.error("Unexpected exception when accessing property [{}]", propertyClass.getName(), e);

• When a suppression is really needed, add @SuppressWarnings("java:S2629") on the enclosing method, together with the comment explaining why the String is built at the call site. Do not use NOSONAR, which suppresses every rule on that line:

  @Override
  @SuppressWarnings("java:S2629")
  public void send(RemoteEventData remoteEvent)
  {
      // Build the String on purpose: log arguments are kept as objects in the captured LogEvent and
      // XStream-serialized into the job log (see SafeMessageConverter in xwiki-commons), and a RemoteEventData
      // is Serializable by design since it is the replicated payload, so it would be written out in full.
      this.logger.debug("Send JGroups remote event [{}]", remoteEvent.toString());
      ...
  }

Imports

• imports should be added individually for each class/interface
• individual imports should be grouped together and separated by blank lines following this model:

  import java.*
  
  import javax.*
  
  import jakarta.*
  
  import org.*
  
  import com.*
  
  import <any other imports>
  
  import static <any static imports>

• The above code style settings for IntelliJ IDEA will automatically follow these rules, so you usually do not have to take care (Be careful that the default configuration for IntelliJ IDEA is not appropriate). For Eclipse you should import eclipse.importorder.

Equals/HashCode and ToString implementations

We've decided to standardize on using Apache Commons Lang HashCodeBuilder, EqualsBuilder and ToStringBuilder.

For example:

...
    @Override
    public boolean equals(Object object)
    {
        if (object == null) {
            return false;
        }
        if (object == this) {
            return true;
        }
        if (object.getClass() != getClass()) {
            return false;
        }
        WikiBotListenerData rhs = (WikiBotListenerData) object;
        return new EqualsBuilder()
            .appendSuper(super.equals(object))
            .append(getReference(), rhs.getReference())
            .isEquals();
    }

    @Override
    public int hashCode()
    {
        return new HashCodeBuilder(3, 17)
            .appendSuper(super.hashCode())
            .append(getReference())
            .toHashCode();
    }
...

XWiki provides a custom ToStringBuilder implementation named XWikiToStringBuilder that uses a custom XWiki's toString style (see the Text Module for information).

For example:

...
    @Override
    public String toString()
    {
        ToStringBuilder builder = new XWikiToStringBuilder(this);
        builder = builder.append("Typed", isTyped())
            .append("Type", getType().getScheme());

        if (getReference() != null) {
            builder = builder.append("Reference", getReference());
        }

        if (!getBaseReferences().isEmpty()) {
            builder = builder.append("Base References", getBaseReferences());
        }

        Map<String, String> params = getParameters();
        if (!params.isEmpty()) {
            builder = builder.append("Parameters", params);
        }

        return builder.toString();
    }
...

This example would generate the following:

...
ResourceReference reference = new ResourceReference("reference", ResourceType.DOCUMENT);
Assert.assertEquals("Typed = [true] Type = [doc] Reference = [reference]", reference.toString());

reference.addBaseReference("baseref1");
reference.addBaseReference("baseref2");
Assert.assertEquals("Typed = [true] Type = [doc] Reference = [reference] "
    + "Base References = [[baseref1], [baseref2]]", reference.toString());

reference.setParameter("name1", "value1");
reference.setParameter("name2", "value2");
Assert.assertEquals("Typed = [true] Type = [doc] Reference = [reference] "
    + "Base References = [[baseref1], [baseref2]] "
    + "Parameters = [[name1] = [value1], [name2] = [value2]]", reference.toString());
...

Test classes

We sometimes write complex tools in test classes but those should never be used in main code. While Maven accept it technically it's not the case of Eclipse for example.

If they really are needed then it's a sign that they should probably move to main or that the test tool manipulating those classes should itself have its classes locate in test (see xwiki-platform-test-page for an example for this use case).

Script Services

See Best practices for the Script Module.

Deprecation

public class Worker
{
    /**
     * Calculate period between versions.
     *
     * @param machine the instance
     * @return the computed time
     * @deprecated This method is no longer acceptable to compute time between versions because... Use {@link Utils#calculatePeriod(Machine)} instead.
     */
    @Deprecated(since = "4.5")
    public int calculate(Machine machine)
    {
        return machine.exportVersions().size() * 10;
    }
}

Optional

Our rules around the Java Optional<> class are:

• Always try to use Optional in return values for methods that can return null and don’t do it if there’s a good-enough reason (to be justified since it deviates from the best practice).
• Don't use Optional in return values of methods when these methods can be used from scripting (Velocity). Until this issue is fixed at least.

Quality check ignores

XWiki executes different quality checks during the build, using different tools (checkstyle, spoon, sonarqube). Sometimes, some of these checks are either false positives or checks that we want to temporarily disable (not a good practice but it may happen). The instructions below provide a best practice approach for doing this.

Checkstyle

• If the issue affects the whole class/interface/etc, use @SuppressWarnings("checkstyle:<rule name here>") on the class/interface/etc.
• If the issue affects a whole method, use @SuppressWarnings("checkstyle:<rule name here>") on the method.
• If the issue affects a variable declaration, use @SuppressWarnings("checkstyle:<rule name here>") on the variable declaration.
• If the issue affects a line (basically whenever an annotation is not allowed), use // CHECKSTYLE:<rule name> to ignore the next line or // CHECKSTYLE:<rule name>(n) to ignore the next n lines.

  Example:

  @Override
  InternalBinaryStringEncoder getEncoder()
  {
      return new AbstractBouncyCastleInternalBinaryStringEncoder(new HexEncoder(), BLOCK_SIZE, CHAR_SIZE)
      {
          @Override
          public boolean isValidEncoding(byte b)
          {
              // Cryptography requires complex expressions, allow a few such expressions
              // CHECKSTYLE:BooleanExpressionComplexity
              return ((b >= 0x2f && b <= 0x39) || (b >= 0x41 && b <= 0x46) || (b >= 0x61 && b <= 0x66));
          }
      };
  }

• Always make sure to add a comment explaining why the ignore is there, using the following formats* When using an annotation:

  // This file has lists of strings copied from a source, making them constants would complicate updating from
  // upstream.
  @SuppressWarnings("checkstyle:MultipleStringLiterals")
  public class HTMLDefinitions
  ...

  • When using a comment:

    // Cryptography requires complex expressions, allow a few such expressions
    // CHECKSTYLE:BooleanExpressionComplexity
    return ((b >= 0x2f && b <= 0x39) || (b >= 0x41 && b <= 0x46) || (b >= 0x61 && b <= 0x66));

SonarQube

• Use the @SuppressWarnings(<rule id>) annotation wherever it’s valid to use it, i.e. everywhere except inside methods (except for variable declarations). Note that <rule id> is the full Sonarqube issue id, e.g. java:S1133.
  • Always add a comment to explain why it's considered a false positive or the reason to ignore the issue, using the format:

    // Explanation here
    @SuppressWarnings("java:S1133")
    public final class ABC
    {
    ...

    Example:

    @Override
    // The token is generated and not user-controlled
    @SuppressWarnings("javasecurity:S5145")
    public boolean isTokenValid(String token)
    {
    ...

• For the places where the @SuppressWarnings annotation is not allowed, continue using the SonarCloud UI for now until SonarQube implements a solution for this.
• Sometimes we also need to disable rules on several classes. For example right now we ignore the java:S1133 rule for all classes in /xwiki-platform-legacy-*//*.java. The rule is to define this in the xwiki-commons/xwiki-commons-pom.xml inside the <properties> section, as in:

  <!-- Tell SonarQube to not report usage of deprecated APIs in XWiki legacy code as it's fine there and in any
       case we don't want to take the risk to perform refactorings on legacy code. -->
  <sonar.issue.ignore.multicriteria>e1</sonar.issue.ignore.multicriteria>
  <sonar.issue.ignore.multicriteria.e1.ruleKey>java:S1133</sonar.issue.ignore.multicriteria.e1.ruleKey>
  <sonar.issue.ignore.multicriteria.e1.resourceKey>
    **/xwiki-*-legacy-*/**/*.java
  </sonar.issue.ignore.multicriteria.e1.resourceKey>

  Information

  When these ignores were declared in the SonarCloud UI, they were applied automatically to all branches, and now that we declare them in our pom.xml we need to merge the changes to all branches or they'll fail on some branches

Don't implement clone() methods

Do not implement Java's Object.clone() method (nor implement the Cloneable interface). This is enforced by SonarQube rule java:S2975.

The clone() / Cloneable mechanism is broadly considered broken: Cloneable doesn't declare a clone() method, the contract is loosely specified, it bypasses constructors, it interacts poorly with final fields, and it makes deep-versus-shallow copy semantics hard to reason about.

Instead, provide a copy constructor or a factory/copy method:

// Bad
public class Foo implements Cloneable
{
    @Override
    public Object clone()
    {
        ...
    }
}

// Good - copy constructor
public class Foo
{
    public Foo(Foo original)
    {
        // Copy the state of "original" into this new instance
    }
}

// Good - factory/copy method
public class Foo
{
    public Foo copy()
    {
        ...
    }
}

For existing clone() methods, progressively deprecate them in favor of a copy constructor or copy method, on a best-effort basis and only where it doesn't break backward compatibility. The existing occurrences reported by SonarQube have been marked as "Accepted".

Nullability annotations

We use JSpecify annotations (org.jspecify.annotations.*) to document nullability in our APIs.

These annotations complement (they do not replace) our Optional guidelines: prefer Optional for return values where it is appropriate, and use nullability annotations for the remaining cases (parameters, fields, and return values where Optional cannot be used, e.g. APIs called from scripting).

Using @Nullable

Annotate with @Nullable any element (method return value, parameter or field) that can legitimately be null:

import org.jspecify.annotations.Nullable;
...
/**
 * @param namespace the namespace for which to check compatibility
 * @return true if compatible, false if incompatible, or null if unknown
 */
@Nullable
Boolean isCompatible(String namespace);

For a parameter that accepts null, place the annotation on the parameter:

public String resolve(String reference, @Nullable List<String> parameters)

Rules:

• Place the annotation on the API contract, i.e. on the interface method (or abstract method) when there is one. The implementation inherits the contract and should not repeat it.
• A single @Nullable is enough. We do not use @CheckForNull: SonarQube treats JSpecify's @Nullable as a "strong" nullable marker, so it will still report an issue (e.g. rule java:S2447 for Boolean methods) when calling code dereferences the value without anull check.

@NullMarked

@NullMarked flips the default inside its scope (package, class or module, typically declared in package-info.java) so that every unannotated type is considered non-null and only @Nullable elements may be null. This is the recommended long-term direction as it makes the absence of an annotation meaningful.

Adopt @NullMarked deliberately, one module at a time, and only once you are ready to annotate all the nullable elements in that scope. Do not blanket-apply it to legacy modules such as xwiki-platform-oldcore.