Convert immutable classes to Java records by Knud-Aage · Pull Request #88 · bitrepository/reference

Knud-Aage · 2026-07-07T10:30:27Z

Converted 10 immutable classes to Java
Deleted equals(), hashCode(), and toString() implementations where Java's auto-generated record versions are equivalent
Updated all call sites to use record accessor names
Added dedicated unit tests for record-specific behaviour: compact constructor validation, non-canonical constructor null-coalescing, custom toString format, and component-based equality

…updated tests

…Metric, JobID and SelectedComponentInfo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ole-v-v

I love how much terser the class declarations have become.

We have lost some Javadoc comments, many of which I tend to find useful and helpful. Please see if you can fit the information into new Javadoc comments on the record classes except where self-evident and/or only written for the sake of there being Javadoc. Also repeated information is often needed only once.

You have made sure to retain some getters with name starting with get (for example SelectedComponentInfo#getID). Fine.
And I am sorry, we need these in all cases. This is for backward compatibility.
I suggest:

In our own code use the new getters as you are generally doing already.
For example in GeneralConversationState#start you call delay.count() instead of delay.getCount().
Also in the last line of the GettingFile constructor I prefer using pillar.componentID() over pillar.getID().
Insert (reinsert) getXx() methods in all new record classes,
and at the same time deprecate them for removal.

Nice work. This will ease reading our code in the future.

ole-v-v · 2026-08-05T09:36:44Z

 */
 package org.bitrepository.client.conversation.selector;

+import org.jetbrains.annotations.NotNull;


I’m in favour of not using Jetbrains-specific annotations. Also when we already use them in a few places (probably by accident).

Changed it to jspecify

ole-v-v · 2026-08-05T09:37:14Z

    @Override
-    public String toString() {
-        return getClass().getSimpleName() + ": componentID=" + componentID + ", componentTopic=" + componentTopic;
+    public @NotNull String toString() {


Annotation/tag is not necessary.

Removed toString

ole-v-v · 2026-08-05T10:31:35Z

    @Override
-    public String toString() {
-        return getClass().getSimpleName() + ": componentID=" + componentID + ", componentTopic=" + componentTopic;
+    public @NotNull String toString() {


I am not perfectly sure whether we want the explicit toString() method at all. The default one yields somthing like SelectedComponentInfo[componentID=some-id, componentTopic=some-topic]. The difference is basically the colon vs. the square brackets. I tend to think it’s close enough that we don’t want to maintain our own implementation any more?

To me it counts that the toString() method never was documented to yields a specific format and that parsing the result from toString() is always discouraged and in this case unnecessary since we got the getters. toString() is there for readable log and trace output, and the default implementation serves that purpose just fine.

Removed toString

ole-v-v · 2026-08-05T10:35:43Z

     * @param outputHandler The {@link OutputHandler} for handling outputting results
     */
-    public CompleteEventAwaiter(Settings settings, OutputHandler outputHandler) {
+    protected CompleteEventAwaiter(Settings settings, OutputHandler outputHandler) {


Since the class is abstract, I guess it’s OK to declare the constructor protected.

ole-v-v · 2026-08-05T10:42:17Z

-    /**
-     * @return The ID of the pillar chosen by this selector if finished. If unfinished null is returned
-     */
    public String getID() {


Yes, unfortunately we need to keep the getters named get-something for backward compatibility (we might have deprecated them, even for removal).

Done it for every changed class to record

ole-v-v · 2026-08-06T09:27:35Z

+    IntegrityModel store,
+    IntegrityAlerter alerter,
+    AuditTrailManager auditManager
+) implements WorkflowContext {}


I agree to leave out the toString() method. Unfortunately we need the old get methods with names starting with get (may deprecate for removal).

Reinserted getters for for every changed class

ole-v-v · 2026-08-06T09:34:21Z

-    protected final String fileID;
-    protected final String checksum;
-    protected final Instant calculationDate;
+public record ChecksumEntry(String fileID, String checksum, Instant calculationInstant) {


Nice rename, thanks.

ole-v-v · 2026-08-06T09:39:29Z

        Assertions.assertEquals(CE_DATE, ce.getCalculationInstant());
    }
+
+    @Test


It’s good to think of new tests.

ole-v-v · 2026-08-06T09:43:33Z

+    @Tag("pillartest")
+    void compactConstructorRejectsNullFileID() {
+        addDescription("The compact constructor must reject a null fileID");
+        Assertions.assertThrows(Exception.class, () -> new ChecksumEntry(null, CE_CHECKSUM, CE_DATE));


Since the constructor is not declared to throw any checked exception, I would prefer to strengthen the assertion to require a RuntimeException (may even require that it is either NullPointerException or IllegalArgumentException, but this would require more code lines and probably isn’t worth it).

Introduced IllegalArgumentException assertions.assertThrow

ole-v-v · 2026-08-06T09:46:40Z

+    @Test
+    @Tag("regressiontest")
+    @Tag("pillartest")
+    void equalityIsComponentBased() {


I tend to think of this test as unneeded and not really one that we want. Does it test anything else than the default, auto-generated equals() and hashCode() methods of a record behave correctly? It’s not our job to test the Java language.

…e to PR review.

…eview.

…ue to backward compatibility issues.

ole-v-v · 2026-08-10T09:57:02Z

-     * The topic for communication with the selected pillar
-     */
-    protected final String componentTopic;
+public record SelectedComponentInfo(@NonNull String componentID, @NonNull String componentTopic) {


I wish I knew whether componentTopic was the right name for this field.

The name used in some places in the old class and agrees with some of the Javadoc comments.

On the other hand ComponentSelector feeds a “reply to” value into the field, and GettingFile uses the value as a destination. Also the name of the getter getDestination() supports this interpretation.

Let it be or add the alternative interpretation to the Javadoc comment, for example “The topic or destination for comm...”.

ole-v-v · 2026-08-10T10:53:28Z

+        Assertions.assertThrows(IllegalArgumentException.class, () -> new ChecksumEntry(null, CE_CHECKSUM, CE_DATE));
+    }


I wasn’t being clear. The current implementation throws IllegalArgumentException, not documented. I would find an implementation throwing NullPointerException in this place perfectly fine too. For example if we replace the use of out own ArgumentValidator with Java’s Objects.requireNonNull(). So IMHO the test should accept either.

Easy fix:

Assertions.assertThrows(RuntimeException.class, () -> new ChecksumEntry(null, CE_CHECKSUM, CE_DATE));

(since both ``IllegalArgumentExceptionandNullPointerException` are subclasses of `RuntimeException`).

Gold-plated test, maybe a bit too complicated and/or long-winded:

RuntimeException exception = Assertions.assertThrows( RuntimeException.class, () -> new ChecksumEntry(null, CE_CHECKSUM, CE_DATE)); Assertions.assertTrue(exception instanceof NullPointerException || exception instanceof IllegalArgumentException, "Expected: NullPointerException or IllegalArgumentException. Actual: " + exception);

Changed for both of the methods that has IllegalArgumentException in them

We don’t need to change compactConstructorRejectsEmptyFileID(). For an empty string IllegalArgumentException is the right exception type. NullPointerException would not make sense.

ole-v-v · 2026-08-10T11:05:30Z

+    }
+
+    @Test
+    void equalityIsComponentBased() {


I suggest leaving out.

…ords-in-java

ole-v-v · 2026-08-11T13:19:46Z

+        RuntimeException exception = Assertions.assertThrows(
+                RuntimeException.class, () -> new ChecksumEntry("", CE_CHECKSUM, CE_DATE));
+        Assertions.assertTrue(exception instanceof NullPointerException || exception instanceof IllegalArgumentException,
+                "Expected: NullPointerException or IllegalArgumentException. Actual: " + exception);


As I said elsewhere, the change in this method is not needed. We prefer it as it was before. It was simpler and a more precise test.

Changed it back to the former

All possible records found in the different classes, implemented and …

510ef31

…updated tests

Knud-Aage requested review from Thommelise and ole-v-v July 7, 2026 10:30

Add record-specific unit tests for CountAndTimeUnit, PillarCollection…

04fb8f6

…Metric, JobID and SelectedComponentInfo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

ole-v-v requested changes Aug 6, 2026

View reviewed changes

Knud-Aage added 9 commits August 7, 2026 11:57

Changed from org.jetbrains.annotations to org.jspecify.annotations du…

17c4e2b

…e to PR review.

Added deprecated getters for componentID

ab350b6

Added JavaDoc according to PR review suggestions

faa688f

Removed toString method due to PR review.

febc83c

Changes due to PR review

fb8d468

Replaced Exception with IllegalArgumentException as suggested by PR r…

de1251d

…eview.

Removed test method equalityIsComponentBased

beb16bc

Reinserted deprecated methods from immutable classes to the records d…

31601b3

…ue to backward compatibility issues.

More changes due to PR review

20c9068

ole-v-v reviewed Aug 10, 2026

View reviewed changes

Comment thread ...rvice/src/test/java/org/bitrepository/integrityservice/cache/PillarCollectionMetricTest.java Outdated

ole-v-v reviewed Aug 10, 2026

View reviewed changes

Knud-Aage added 2 commits August 11, 2026 13:43

Merge remote-tracking branch 'origin/master' into BITMAG-1256-use-rec…

fac1098

…ords-in-java

Changes due to PR review

bc4e999

ole-v-v reviewed Aug 11, 2026

View reviewed changes

Changes due to PR review

f66504d

		Assertions.assertThrows(IllegalArgumentException.class, () -> new ChecksumEntry(null, CE_CHECKSUM, CE_DATE));
		}

Uh oh!

Conversation

Knud-Aage commented Jul 7, 2026

Uh oh!

ole-v-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ole-v-v Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ole-v-v Aug 10, 2026 •

edited

Loading