RLPNC-9054: Changes to record-matching API for multivalued fields - #318
RLPNC-9054: Changes to record-matching API for multivalued fields#318ethteck wants to merge 1 commit into
Conversation
seth-mg
left a comment
There was a problem hiding this comment.
The note on copyrights applies to more than just the one flagged file.
I ran the PR through Sonar and the results are available here. There are no Blocker or Critical issues, so you can accept or ignore as you see fit.
Here's a co-pilot review. The Blocker item in the list is understood, but we'll repeat it for the sake of thoroughness. Addressing the High and Medium issues seems worthwhile. If you agree, please address.
Blocker: public API break in record similarity field types
model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/NameField.java:36,56,66model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/DateField.java:34,54,64model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/AddressField.java:35,54,65model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/StringField.java:27-28model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/NumberField.java:27-28model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/BooleanField.java:27-28- usage change example:
examples/src/main/java/com/basistech/rosette/examples/RecordSimilarityExample.java:75-123
This changes the public construction model from scalar/subtype instances (e.g. NameField.FieldedName) to wrapper-with-list (NameField.builder().data(List.of(...))). Existing client code targeting master will fail to compile and likely break binary compatibility.
Requested fix: keep backward-compatible constructors/factories (or adapters) for existing single-value call sites, deprecate old entry points if needed, and layer multivalue support without breaking current consumers.
High: potential NPE in toJson() when data is null
model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/NameField.java:43model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/DateField.java:41model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/AddressField.java:42model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/StringField.java:32model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/NumberField.java:32model/src/main/java/com/basistech/rosette/apimodel/recordsimilarity/records/BooleanField.java:32
toJson() dereferences data.size() without a null guard. With Lombok builders, @NotEmpty does not enforce non-null at build time by itself, so builder().build() can produce data == null and fail at serialization.
Requested fix: enforce non-null in construction (explicit validation in constructor/builder) or fail fast with a clear exception before serialization.
Medium: missing explicit tests for true multi-element field values
json/src/test/java/com/basistech/rosette/apimodel/RecordSimilarityRequestTest.java:294-313json/src/test/java/com/basistech/rosette/apimodel/recordsimilarity/RecordSimilarityResponseTest.java:84-89
Current test updates mostly wrap single values in List.of(...) (e.g. one string/one bool/one number). I don’t see coverage for fields containing multiple values in one field payload (the core behavior introduced by this change).
Requested fix: add request/response round-trip tests with multi-element data arrays (and mixed structured/unstructured entries where allowed), plus negative cases for malformed array elements.
| @@ -0,0 +1,48 @@ | |||
| /* | |||
| * Copyright 2025 Basis Technology Corp. | |||
There was a problem hiding this comment.
The new entity for copyright headers is 'Babel Street Rosette Ltd'
No description provided.