CPD: remove the duplicated sensor record and response code, gate cpd-check (#115)

AGENTS.md

@@ -6,7 +6,7 @@ The code is formatted with the MetricsHub Eclipse formatter profile (`metricshub
 
 Checkstyle (`checkstyle.xml`, byte-identical to jawk's) runs at `verify` with `failOnViolation=true`. Fix violations rather than suppressing them; when a suppression is justified, wrap the code in `// CHECKSTYLE.OFF: <RuleName>` / `// CHECKSTYLE.ON: <RuleName>` comments.
 
-All Java source files under `src/main/java` must include the proper LGPL-3 license header (the `license-maven-plugin` check covers `main/java/**/*.java` only; tests, Markdown and resources carry no header). When you add a new source file, run `mvn license:update-file-header` before committing (and before building, since the build fails if a source file lacks the header).
+All Java source files under `src/main/java` must include the proper LGPL-3 license header (the `license-maven-plugin` check covers `main/java/**/*.java` only; tests, Markdown and resources carry no header). When you add a new source file, run `mvn license:update-file-header` before committing (and before building, since the build fails if a source file lacks the header). The plugin only adds missing headers; it is configured never to rewrite existing ones, so do not edit copyright lines in bulk. The library is a fork of the Verax Systems IPMI Library for Java: files derived from Verax code (`org.metricshub.ipmi.core`) say `Copyright 2023 Verax Systems, MetricsHub`, files written by MetricsHub say `Copyright 2023 MetricsHub`. A new file gets a MetricsHub copyright line; if it contains code moved from a Verax-derived file, give it that file's copyright line.
 
 All public methods must have proper Javadoc. Check the output of Maven to identify issues with Javadoc and fix these issues.
 
@@ -26,7 +26,7 @@ Unit tests must not depend on a real BMC. Exercising the RMCP+ session code agai
 
 ## Code quality reports
 
-Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD and SpotBugs are not yet gated (issues #115 and #116 track the clean-up). Do not add new violations: check the reports for the files you changed before committing and submitting your code. On JDK 21+ the SpotBugs plugin version inherited from the parent POM cannot read the JDK class files; run `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs` instead.
+Code quality reports (checkstyle, pmd/cpd, spotbugs) are generated by `mvn verify site` into ./target/checkstyle-result.xml, ./target/pmd.xml, ./target/cpd.xml and ./target/spotbugsXml.xml. Checkstyle and PMD are gated (the build fails on any error); CPD is gated on duplications of 100 tokens or more (`cpd-check` at `verify`), while the site report lists those of 50 tokens or more; SpotBugs is not yet gated (issue #116 tracks the clean-up). Extract shared code instead of copying it: `AbstractSensorRecord` holds what the Full, Compact and Event-Only sensor records share, and `IpmiCommandCoder.validateResponse()` the response checks of every command. Do not add new violations: check the reports for the files you changed before committing and submitting your code. On JDK 21+ the SpotBugs plugin version inherited from the parent POM cannot read the JDK class files; run `mvn com.github.spotbugs:spotbugs-maven-plugin:4.10.4.1:spotbugs` instead.
 
 ## Documentation
 

README.md

@@ -12,7 +12,7 @@ The IPMI Java Client is a library that communicates with the IPMI host, fetches
 
 ## Upgrading
 
-Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged.
+Version 1.2.03 makes the `protected` fields of the protocol classes (`AbstractIpmiRunner`, `MessageHandler`, `IpmiLanMessage`, `ConfidentialityAlgorithm`, `IntegrityAlgorithm`) `private`. Subclasses must use the new `protected` accessors instead; see [Upgrading from 1.2.02](https://metricshub.org/ipmi-java/#upgrading-from-1-2-02) for the list. The `IpmiClient` API is unchanged. The Full, Compact and Event-Only sensor records now share the `AbstractSensorRecord` superclass, and commands can check responses with `IpmiCommandCoder.validateResponse()`; both are described on the same page.
 
 ## Build instructions
 
@@ -54,8 +54,10 @@ But it is strongly recommended to only use [GitHub Actions "Release to Maven Cen
 
 ## License
 
-License is GNU General Lesser Public License (LGPL) version 3.0. Each source file includes the LGPL-3 header (build will fail otherwise).
-To update source files with the proper header, simply execute the below command:
+License is GNU General Lesser Public License (LGPL) version 3.0. The IPMI Library for Java by Verax Systems is published under the GNU GPL v3; this fork uses it under a commercial (non-GPL) license granted by Verax Systems to Sentry Software in 2021. The source files derived from it keep Verax Systems as copyright holder.
+
+Each source file includes the LGPL-3 header (build will fail otherwise).
+To add the header to new source files, simply execute the below command (existing headers are never modified):
 
 ```bash
 mvn license:update-file-header

pom.xml

@@ -148,13 +148,14 @@
 				</executions>
 			</plugin>
 
-			<!-- pmd: fail the build on any violation of pmd.xml, so the PMD report stays clean (cpd-check awaits #115) -->
+			<!-- pmd: fail the build on any violation of pmd.xml or any duplication of 100+ tokens (the CPD report lists 50+) -->
 			<plugin>
 				<groupId>org.apache.maven.plugins</groupId>
 				<artifactId>maven-pmd-plugin</artifactId>
 				<version>3.28.0</version>
 				<configuration>
 					<targetJdk>${maven.compiler.release}</targetJdk>
+					<minimumTokens>100</minimumTokens>
 					<rulesets>
 						<ruleset>pmd.xml</ruleset>
 					</rulesets>
@@ -164,17 +165,23 @@
 					<execution>
 						<goals>
 							<goal>check</goal>
+							<goal>cpd-check</goal>
 						</goals>
 					</execution>
 				</executions>
 			</plugin>
 
-			<!-- license -->
+			<!-- license: add headers to new files, never rewrite existing ones. The parent's canUpdateCopyright would turn
+				every "Copyright 2023 Verax Systems, MetricsHub" into "Copyright 2023 - <current year> MetricsHub", and its
+				canUpdateDescription makes every header look outdated on Windows (CRLF vs LF), which re-renders the copyright
+				line as "Copyright (C) ..." -->
 			<plugin>
 				<groupId>org.codehaus.mojo</groupId>
 				<artifactId>license-maven-plugin</artifactId>
 				<configuration>
 					<licenseName>lgpl_v3</licenseName>
+					<canUpdateCopyright>false</canUpdateCopyright>
+					<canUpdateDescription>false</canUpdateDescription>
 				</configuration>
 			</plugin>
 		</plugins>

src/main/java/org/metricshub/ipmi/client/model/Sensor.java

@@ -23,6 +23,7 @@
  */
 
 import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData;
+import org.metricshub.ipmi.core.coding.commands.sdr.record.AbstractSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.EntityId;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord;
@@ -70,21 +71,12 @@ public boolean isFull() {
 	}
 
 	/**
-	 * Cast the current record to a {@link CompactSensorRecord}
+	 * Cast the current record to the {@link AbstractSensorRecord} shared by the Compact and Full sensor records
 	 *
-	 * @return {@link CompactSensorRecord} instance
+	 * @return {@link AbstractSensorRecord} instance, or <code>null</code> if the record is neither Compact nor Full
 	 */
-	private CompactSensorRecord getCompactSensorRecord() {
-		return (CompactSensorRecord) sensorRecord;
-	}
-
-	/**
-	 * Cast the current record to a {@link FullSensorRecord}
-	 *
-	 * @return {@link FullSensorRecord} instance
-	 */
-	private FullSensorRecord getFullSensorRecord() {
-		return (FullSensorRecord) sensorRecord;
+	private AbstractSensorRecord getSensorRecord() {
+		return isCompact() || isFull() ? (AbstractSensorRecord) sensorRecord : null;
 	}
 
 	/**
@@ -93,13 +85,8 @@ private FullSensorRecord getFullSensorRecord() {
 	 * @return {@link EntityId} instance
 	 */
 	public EntityId getEntityId() {
-
-		if (isCompact()) {
-			return getCompactSensorRecord().getEntityId();
-		} else if (isFull()) {
-			return getFullSensorRecord().getEntityId();
-		}
-		return null;
+		final AbstractSensorRecord record = getSensorRecord();
+		return record == null ? null : record.getEntityId();
 	}
 
 	/**
@@ -108,12 +95,8 @@ public EntityId getEntityId() {
 	 * @return {@link Byte} value or <code>null</code> if the type of the sensor cannot be detected.
 	 */
 	public Byte getDeviceId() {
-		if (isCompact()) {
-			return getCompactSensorRecord().getEntityInstanceNumber();
-		} else if (isFull()) {
-			return getFullSensorRecord().getEntityInstanceNumber();
-		}
-		return null;
+		final AbstractSensorRecord record = getSensorRecord();
+		return record == null ? null : record.getEntityInstanceNumber();
 	}
 
 	/**
@@ -122,12 +105,8 @@ public Byte getDeviceId() {
 	 * @return {@link String} value
 	 */
 	public String getName() {
-		if (isCompact()) {
-			return getCompactSensorRecord().getName();
-		} else if (isFull()) {
-			return getFullSensorRecord().getName();
-		}
-		return null;
+		final AbstractSensorRecord record = getSensorRecord();
+		return record == null ? null : record.getName();
 	}
 
 	/**

src/main/java/org/metricshub/ipmi/client/runner/GetSensorsRunner.java

@@ -36,6 +36,7 @@
 import org.metricshub.ipmi.core.coding.commands.sdr.GetSensorReadingResponseData;
 import org.metricshub.ipmi.core.coding.commands.sdr.ReserveSdrRepository;
 import org.metricshub.ipmi.core.coding.commands.sdr.ReserveSdrRepositoryResponseData;
+import org.metricshub.ipmi.core.coding.commands.sdr.record.AbstractSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.CompactSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.FullSensorRecord;
 import org.metricshub.ipmi.core.coding.commands.sdr.record.ReadingType;
@@ -84,8 +85,11 @@ public List<Sensor> call() throws Exception {
 				// repository (see #getSensorData for details).
 				sensorRecord = super.getSensorData(reservationId);
 
+				// Only Full and Compact sensor records have a reading associated
+				// with them (see IPMI specification for details)
 				if (sensorRecord instanceof FullSensorRecord || sensorRecord instanceof CompactSensorRecord) {
-					int recordReadingId = getReadingId(sensorRecord);
+					int recordReadingId = TypeConverter
+							.byteToInt(((AbstractSensorRecord) sensorRecord).getSensorNumber());
 
 					// If our record has got a reading associated, we get request
 					// for it
@@ -138,38 +142,15 @@ static String buildStates(final GetSensorReadingResponseData data, final SensorR
 		}
 
 		try {
-			final String deviceName;
-			final List<ReadingType> events;
-			if (sensorRecord instanceof CompactSensorRecord) {
+			final AbstractSensorRecord record = (AbstractSensorRecord) sensorRecord;
+			final String deviceName = record.getName();
 
-				CompactSensorRecord compactSensorRecord = (CompactSensorRecord) sensorRecord;
-
-				deviceName = compactSensorRecord.getName();
-
-				if (compactSensorRecord.getEventReadingType() == OEM_EVENT_READING_TYPE) {
-					return buildOemState(data.getRaw(), deviceName);
-				}
-
-				events = data
-						.getStatesAsserted(
-								compactSensorRecord.getSensorType(),
-								compactSensorRecord.getEventReadingType());
-			} else {
-
-				FullSensorRecord fullSensorRecord = (FullSensorRecord) sensorRecord;
-
-				deviceName = fullSensorRecord.getName();
-
-				if (fullSensorRecord.getEventReadingType() == OEM_EVENT_READING_TYPE) {
-					return buildOemState(data.getRaw(), deviceName);
-				}
-
-				events = data
-						.getStatesAsserted(
-								fullSensorRecord.getSensorType(),
-								fullSensorRecord.getEventReadingType());
+			if (record.getEventReadingType() == OEM_EVENT_READING_TYPE) {
+				return buildOemState(data.getRaw(), deviceName);
 			}
 
+			final List<ReadingType> events = data.getStatesAsserted(record.getSensorType(), record.getEventReadingType());
+
 			return appendReadingTypes(events, deviceName);
 
 		} catch (Exception e) {
@@ -252,26 +233,4 @@ private GetSensorReadingResponseData getSensorRecordReading(final int recordRead
 		}
 		return null;
 	}
-
-	/**
-	 * Get the reading id which is required by the BMC to answer reading commands.
-	 *
-	 * @param sensorRecord {@link SensorRecord} instance expected as Full or Compact.
-	 * @return The sensor number of the record otherwise -1 if cannot determine the record type.
-	 */
-	private int getReadingId(final SensorRecord sensorRecord) {
-		// We check if the received record is either FullSensorRecord or
-		// CompactSensorRecord, since these types have readings
-		// associated with them (see IPMI specification for details).
-		if (sensorRecord instanceof FullSensorRecord) {
-			FullSensorRecord fsr = (FullSensorRecord) sensorRecord;
-			return TypeConverter.byteToInt(fsr.getSensorNumber());
-
-		} else if (sensorRecord instanceof CompactSensorRecord) {
-			CompactSensorRecord csr = (CompactSensorRecord) sensorRecord;
-			return TypeConverter.byteToInt(csr.getSensorNumber());
-		}
-
-		return -1;
-	}
 }

src/main/java/org/metricshub/ipmi/core/coding/commands/IpmiCommandCoder.java

@@ -23,8 +23,10 @@
  */
 
 import org.metricshub.ipmi.core.coding.PayloadCoder;
+import org.metricshub.ipmi.core.coding.payload.CompletionCode;
 import org.metricshub.ipmi.core.coding.payload.IpmiPayload;
 import org.metricshub.ipmi.core.coding.payload.PlainMessage;
+import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse;
 import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction;
 import org.metricshub.ipmi.core.coding.protocol.AuthenticationType;
@@ -75,6 +77,28 @@ public boolean isCommandResponse(IpmiMessage message) {
 		}
 	}
 
+	/**
+	 * Checks that the message is a successful response to this command.
+	 *
+	 * @param message {@link IpmiMessage} wrapping the IPMI response
+	 * @return the IPMI command data of the response
+	 * @throws IllegalArgumentException when the message is not an IPMI LAN response to this command
+	 * @throws IPMIException when the completion code of the response is not {@link CompletionCode#Ok}
+	 */
+	protected byte[] validateResponse(IpmiMessage message) throws IPMIException {
+		if (!isCommandResponse(message)) {
+			throw new IllegalArgumentException("This is not a response for " + getClass().getSimpleName() + " command");
+		}
+		if (!(message.getPayload() instanceof IpmiLanResponse)) {
+			throw new IllegalArgumentException("Invalid response payload");
+		}
+		IpmiLanResponse response = (IpmiLanResponse) message.getPayload();
+		if (response.getCompletionCode() != CompletionCode.Ok) {
+			throw new IPMIException(response.getCompletionCode());
+		}
+		return response.getIpmiCommandData();
+	}
+
 	/**
 	 * Retrieves command code specific for command represented by this class
 	 *

src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/ChassisControl.java

@@ -29,11 +29,9 @@
 import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder;
 import org.metricshub.ipmi.core.coding.commands.IpmiVersion;
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
-import org.metricshub.ipmi.core.coding.payload.CompletionCode;
 import org.metricshub.ipmi.core.coding.payload.IpmiPayload;
 import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanRequest;
-import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse;
 import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction;
 import org.metricshub.ipmi.core.coding.protocol.AuthenticationType;
 import org.metricshub.ipmi.core.coding.protocol.IpmiMessage;
@@ -96,18 +94,8 @@ public ResponseData getResponseData(IpmiMessage message)
 			throws IPMIException,
 			NoSuchAlgorithmException,
 			InvalidKeyException {
-		if (!isCommandResponse(message)) {
-			throw new IllegalArgumentException(
-					"This is not a response for Get Chassis Status command");
-		}
-		if (!(message.getPayload() instanceof IpmiLanResponse)) {
-			throw new IllegalArgumentException("Invalid response payload");
-		}
-		if (((IpmiLanResponse) message.getPayload()).getCompletionCode() != CompletionCode.Ok) {
-			throw new IPMIException(
-					((IpmiLanResponse) message.getPayload())
-							.getCompletionCode());
-		}
+		validateResponse(message);
+
 		return new ChassisControlResponseData();
 	}
 

src/main/java/org/metricshub/ipmi/core/coding/commands/chassis/GetChassisStatus.java

@@ -29,11 +29,9 @@
 import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder;
 import org.metricshub.ipmi.core.coding.commands.IpmiVersion;
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
-import org.metricshub.ipmi.core.coding.payload.CompletionCode;
 import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanMessage;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanRequest;
-import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse;
 import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction;
 import org.metricshub.ipmi.core.coding.protocol.AuthenticationType;
 import org.metricshub.ipmi.core.coding.protocol.IpmiMessage;
@@ -91,20 +89,7 @@ public ResponseData getResponseData(IpmiMessage message)
 			throws IPMIException,
 			NoSuchAlgorithmException,
 			InvalidKeyException {
-		if (!isCommandResponse(message)) {
-			throw new IllegalArgumentException(
-					"This is not a response for Get Chassis Status command");
-		}
-		if (!(message.getPayload() instanceof IpmiLanResponse)) {
-			throw new IllegalArgumentException("Invalid response payload");
-		}
-		if (((IpmiLanResponse) message.getPayload()).getCompletionCode() != CompletionCode.Ok) {
-			throw new IPMIException(
-					((IpmiLanResponse) message.getPayload())
-							.getCompletionCode());
-		}
-
-		byte[] raw = message.getPayload().getIpmiCommandData();
+		byte[] raw = validateResponse(message);
 
 		if (raw == null || (raw.length != 3 && raw.length != 4)) {
 			throw new IllegalArgumentException(

src/main/java/org/metricshub/ipmi/core/coding/commands/fru/GetFruInventoryAreaInfo.java

@@ -29,11 +29,9 @@
 import org.metricshub.ipmi.core.coding.commands.IpmiCommandCoder;
 import org.metricshub.ipmi.core.coding.commands.IpmiVersion;
 import org.metricshub.ipmi.core.coding.commands.ResponseData;
-import org.metricshub.ipmi.core.coding.payload.CompletionCode;
 import org.metricshub.ipmi.core.coding.payload.IpmiPayload;
 import org.metricshub.ipmi.core.coding.payload.lan.IPMIException;
 import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanRequest;
-import org.metricshub.ipmi.core.coding.payload.lan.IpmiLanResponse;
 import org.metricshub.ipmi.core.coding.payload.lan.NetworkFunction;
 import org.metricshub.ipmi.core.coding.protocol.AuthenticationType;
 import org.metricshub.ipmi.core.coding.protocol.IpmiMessage;
@@ -100,20 +98,7 @@ public ResponseData getResponseData(IpmiMessage message)
 			NoSuchAlgorithmException,
 			InvalidKeyException {
 
-		if (!isCommandResponse(message)) {
-			throw new IllegalArgumentException(
-					"This is not a response for Get FRU Inventory Info command");
-		}
-		if (!(message.getPayload() instanceof IpmiLanResponse)) {
-			throw new IllegalArgumentException("Invalid response payload");
-		}
-		if (((IpmiLanResponse) message.getPayload()).getCompletionCode() != CompletionCode.Ok) {
-			throw new IPMIException(
-					((IpmiLanResponse) message.getPayload())
-							.getCompletionCode());
-		}
-
-		byte[] raw = message.getPayload().getIpmiCommandData();
+		byte[] raw = validateResponse(message);
 
 		if (raw == null || raw.length != 3) {
 			throw new IllegalArgumentException(