Skip to content

Commit 29eb528

Browse files
kazemekkazemek
committed
fix(jackson3): address CodeRabbit review on domain mapping
Omit empty Optional attributes, fail early on missing accessors and reserved member names, and make mixed to-many diagnostics order-independent.
1 parent a0b3527 commit 29eb528

7 files changed

Lines changed: 144 additions & 33 deletions

File tree

‎jsonapi-java-jackson3/README.md‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,8 @@ String json = JsonApiJackson3.writer(callerMapper).writeValueAsString(doc);
4242
Custom identifier conversion:
4343

4444
```java
45-
IdentifierConverter prefixer = idValue -> "urn:" + idValue.toString();
45+
IdentifierConverter prefixer =
46+
idValue -> idValue == null ? null : "urn:" + idValue.toString();
4647
JsonApiResourceMapper mapper = JsonApiJackson3.resourceMapper(callerMapper, prefixer);
4748
```
4849

‎jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/MappingDiagnostic.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ public enum MappingDiagnostic {
77
INVALID_RESOURCE_TYPE,
88
INVALID_ATTRIBUTE_NAME,
99
INVALID_RELATIONSHIP_NAME,
10+
MISSING_ACCESSOR,
1011
MISSING_IDENTIFIER,
1112
MISSING_RESOURCE_ANNOTATION,
1213
UNSUPPORTED_ATTRIBUTE_VALUE,

‎jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/DomainResourceWriter.java‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,11 @@ private Attributes buildAttributes(Object resource, ResourceMapping mapping) {
9191
}
9292
Map<String, Object> attributes = new LinkedHashMap<>();
9393
for (MappingProperty property : mapping.attributes()) {
94-
Object value = readValue(resource, property, PropertyRole.ATTRIBUTE);
94+
Object rawValue = readValue(resource, property, PropertyRole.ATTRIBUTE);
95+
if (rawValue instanceof Optional<?> optional && optional.isEmpty()) {
96+
continue;
97+
}
98+
Object value = unwrapOptional(rawValue);
9599
attributes.put(property.jsonapiName(), convertAttributeValue(value));
96100
}
97101
return Attributes.ofAttributes(attributes);
@@ -144,26 +148,33 @@ private RelationshipData extractToManyLinkage(@Nullable Object value, JavaType p
144148
if (items.isEmpty()) {
145149
return RelationshipData.IdentifierCollectionLinkage.empty();
146150
}
147-
Object sample = null;
151+
boolean hasResourceIdentifier = false;
152+
Object firstNonResourceIdentifier = null;
148153
for (Object item : items) {
149-
if (item != null) {
150-
sample = item;
151-
break;
154+
if (item == null) {
155+
continue;
156+
}
157+
if (item instanceof ResourceIdentifier) {
158+
hasResourceIdentifier = true;
159+
} else if (firstNonResourceIdentifier == null) {
160+
firstNonResourceIdentifier = item;
152161
}
153162
}
154-
if (sample instanceof ResourceIdentifier) {
163+
if (hasResourceIdentifier && firstNonResourceIdentifier != null) {
164+
throw new JsonApiMappingException(
165+
MappingDiagnostic.UNSUPPORTED_RELATIONSHIP_VALUE,
166+
firstNonResourceIdentifier.getClass(),
167+
null,
168+
"Mixed element types in to-many relationship collection: expected ResourceIdentifier, got "
169+
+ firstNonResourceIdentifier.getClass().getName());
170+
}
171+
if (hasResourceIdentifier) {
155172
List<ResourceIdentifier> identifiers = new ArrayList<>();
156173
for (Object item : items) {
157-
if (item == null) continue;
158-
if (!(item instanceof ResourceIdentifier resourceIdentifier)) {
159-
throw new JsonApiMappingException(
160-
MappingDiagnostic.UNSUPPORTED_RELATIONSHIP_VALUE,
161-
item.getClass(),
162-
null,
163-
"Mixed element types in to-many relationship collection: expected ResourceIdentifier, got "
164-
+ item.getClass().getName());
174+
if (item == null) {
175+
continue;
165176
}
166-
identifiers.add(resourceIdentifier);
177+
identifiers.add((ResourceIdentifier) item);
167178
}
168179
return new RelationshipData.IdentifierCollectionLinkage(identifiers);
169180
}
@@ -179,7 +190,9 @@ private RelationshipData extractToManyLinkage(@Nullable Object value, JavaType p
179190
checkResourceAnnotation(elementClass);
180191
List<ResourceIdentifier> identifiers = new ArrayList<>();
181192
for (Object item : items) {
182-
if (item == null) continue;
193+
if (item == null) {
194+
continue;
195+
}
183196
identifiers.add(extractIdentifier(item));
184197
}
185198
return new RelationshipData.IdentifierCollectionLinkage(identifiers);
@@ -252,7 +265,7 @@ private static boolean isToManyType(JavaType type) {
252265
if (type.isCollectionLikeType()) {
253266
return true;
254267
}
255-
return type.hasRawClass(Iterable.class);
268+
return type.isTypeOrSubTypeOf(Iterable.class);
256269
}
257270

258271
private static @Nullable JavaType resolveContentType(JavaType type) {

‎jsonapi-java-jackson3/src/main/java/io/github/kazemek/jsonapi/jackson3/internal/MappingDefinitionResolver.java‎

Lines changed: 31 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import io.github.kazemek.jsonapi.annotation.JsonApiId;
55
import io.github.kazemek.jsonapi.annotation.JsonApiRelationship;
66
import io.github.kazemek.jsonapi.annotation.JsonApiResource;
7+
import io.github.kazemek.jsonapi.core.model.JsonApiMembers;
78
import io.github.kazemek.jsonapi.core.validation.MemberNames;
89
import io.github.kazemek.jsonapi.jackson3.JsonApiMappingException;
910
import io.github.kazemek.jsonapi.jackson3.MappingDiagnostic;
@@ -89,6 +90,13 @@ private static void classifyProperties(
8990
for (BeanPropertyDefinition propertyDefinition : propertyDefinitions) {
9091
AnnotatedMember accessor = propertyDefinition.getAccessor();
9192
if (accessor == null) {
93+
if (hasExplicitRoleAnnotation(propertyDefinition)) {
94+
throw new JsonApiMappingException(
95+
MappingDiagnostic.MISSING_ACCESSOR,
96+
rawType,
97+
propertyDefinition.getName(),
98+
"Annotated property '" + propertyDefinition.getName() + "' has no readable accessor");
99+
}
92100
continue;
93101
}
94102
String logicalName = propertyDefinition.getName();
@@ -107,6 +115,12 @@ private static void classifyProperties(
107115
}
108116
}
109117

118+
private static boolean hasExplicitRoleAnnotation(BeanPropertyDefinition propertyDefinition) {
119+
return hasAnnotationAnywhere(propertyDefinition, JsonApiId.class)
120+
|| hasAnnotationAnywhere(propertyDefinition, JsonApiAttribute.class)
121+
|| hasAnnotationAnywhere(propertyDefinition, JsonApiRelationship.class);
122+
}
123+
110124
private static PropertyRole resolveRole(
111125
BeanPropertyDefinition propertyDefinition, String logicalName, Class<?> rawType) {
112126
boolean hasIdentifier = hasAnnotationAnywhere(propertyDefinition, JsonApiId.class);
@@ -159,20 +173,23 @@ private static void validateJsonApiName(
159173
PropertyRole role,
160174
BeanPropertyDefinition propertyDefinition,
161175
Class<?> rawType) {
162-
if (!MemberNames.isValid(jsonapiName)) {
163-
MappingDiagnostic diagnostic =
164-
switch (role) {
165-
case ATTRIBUTE -> MappingDiagnostic.INVALID_ATTRIBUTE_NAME;
166-
case RELATIONSHIP -> MappingDiagnostic.INVALID_RELATIONSHIP_NAME;
167-
default -> null;
168-
};
169-
if (diagnostic != null) {
170-
throw new JsonApiMappingException(
171-
diagnostic,
172-
rawType,
173-
propertyDefinition.getName(),
174-
"Invalid JSON:API member name: " + jsonapiName);
175-
}
176+
MappingDiagnostic diagnostic =
177+
switch (role) {
178+
case ATTRIBUTE -> MappingDiagnostic.INVALID_ATTRIBUTE_NAME;
179+
case RELATIONSHIP -> MappingDiagnostic.INVALID_RELATIONSHIP_NAME;
180+
default -> null;
181+
};
182+
if (diagnostic == null) {
183+
return;
184+
}
185+
if (!MemberNames.isValid(jsonapiName)
186+
|| JsonApiMembers.ID.equals(jsonapiName)
187+
|| JsonApiMembers.TYPE.equals(jsonapiName)) {
188+
throw new JsonApiMappingException(
189+
diagnostic,
190+
rawType,
191+
propertyDefinition.getName(),
192+
"Invalid JSON:API member name: " + jsonapiName);
176193
}
177194
}
178195

‎jsonapi-java-jackson3/src/test/groovy/io/github/kazemek/jsonapi/jackson3/DomainResourceWriterDiagnosticsSpec.groovy‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,4 +255,67 @@ class DomainResourceWriterDiagnosticsSpec extends Specification {
255255
def ex = thrown(JsonApiMappingException)
256256
ex.diagnostic() == MappingDiagnostic.NAME_COLLISION
257257
}
258+
259+
@JsonApiResource(type = "reserved-attr")
260+
static class ReservedAttrNameEntity {
261+
@JsonApiId String id
262+
@JsonApiAttribute(name = "type") String value
263+
}
264+
265+
def "reserved attribute name type throws INVALID_ATTRIBUTE_NAME"() {
266+
given:
267+
def mapper = JsonApiJackson3.resourceMapper(JsonMapper.builder().build())
268+
def entity = new ReservedAttrNameEntity(id: "1", value: "v")
269+
270+
when:
271+
mapper.toResource(entity)
272+
273+
then:
274+
def ex = thrown(JsonApiMappingException)
275+
ex.diagnostic() == MappingDiagnostic.INVALID_ATTRIBUTE_NAME
276+
}
277+
278+
@JsonApiResource(type = "reserved-rel")
279+
static class ReservedRelNameEntity {
280+
@JsonApiId String id
281+
@JsonApiRelationship(name = "id") String other
282+
}
283+
284+
def "reserved relationship name id throws INVALID_RELATIONSHIP_NAME"() {
285+
given:
286+
def mapper = JsonApiJackson3.resourceMapper(JsonMapper.builder().build())
287+
def entity = new ReservedRelNameEntity(id: "1", other: "o")
288+
289+
when:
290+
mapper.toResource(entity)
291+
292+
then:
293+
def ex = thrown(JsonApiMappingException)
294+
ex.diagnostic() == MappingDiagnostic.INVALID_RELATIONSHIP_NAME
295+
}
296+
297+
@JsonApiResource(type = "write-only")
298+
static class MissingAccessorEntity {
299+
@JsonApiId String id
300+
private String secret
301+
302+
@JsonApiAttribute
303+
void setSecret(String secret) {
304+
this.secret = secret
305+
}
306+
}
307+
308+
def "annotated property without readable accessor throws MISSING_ACCESSOR"() {
309+
given:
310+
def mapper = JsonApiJackson3.resourceMapper(JsonMapper.builder().build())
311+
def entity = new MissingAccessorEntity(id: "1")
312+
entity.setSecret("hidden")
313+
314+
when:
315+
mapper.toResource(entity)
316+
317+
then:
318+
def ex = thrown(JsonApiMappingException)
319+
ex.diagnostic() == MappingDiagnostic.MISSING_ACCESSOR
320+
}
258321
}

‎jsonapi-java-jackson3/src/test/groovy/io/github/kazemek/jsonapi/jackson3/ResourceMappingJacksonFeaturesSpec.groovy‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ class ResourceMappingJacksonFeaturesSpec extends Specification {
154154

155155
then:
156156
resource.attributes().attributes().title == "Title"
157+
!resource.attributes().attributes().containsKey("subtitle")
157158
}
158159

159160
def "array to-many relationship produces collection linkage"() {
@@ -270,6 +271,20 @@ class ResourceMappingJacksonFeaturesSpec extends Specification {
270271
ex.diagnostic() == MappingDiagnostic.UNSUPPORTED_RELATIONSHIP_VALUE
271272
}
272273

274+
def "mixed element types with unsupported element first throw UNSUPPORTED_RELATIONSHIP_VALUE"() {
275+
given:
276+
def mapper = JsonApiJackson3.resourceMapper(JsonMapper.builder().build())
277+
def ri = new io.github.kazemek.jsonapi.core.model.ResourceIdentifier("comments", "1", null, null, Map.of())
278+
def entity = new MixedRelEntity(id: "1", items: [new Object(), ri])
279+
280+
when:
281+
mapper.toResource(entity)
282+
283+
then:
284+
def ex = thrown(JsonApiMappingException)
285+
ex.diagnostic() == MappingDiagnostic.UNSUPPORTED_RELATIONSHIP_VALUE
286+
}
287+
273288
def "leading null in to-many ResourceIdentifier collection produces correct linkage"() {
274289
given:
275290
def mapper = JsonApiJackson3.resourceMapper(JsonMapper.builder().build())

‎jsonapi-java-jackson3/src/test/java/io/github/kazemek/jsonapi/jackson3/testmodel/Comment.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
package io.github.kazemek.jsonapi.jackson3.testmodel;
22

33
import io.github.kazemek.jsonapi.annotation.JsonApiId;
4+
import io.github.kazemek.jsonapi.annotation.JsonApiRelationship;
45
import io.github.kazemek.jsonapi.annotation.JsonApiResource;
56

67
@JsonApiResource(type = "comments")
7-
public record Comment(@JsonApiId String id, String body, Person author) {}
8+
public record Comment(@JsonApiId String id, String body, @JsonApiRelationship Person author) {}

0 commit comments

Comments
 (0)