From dd44dac6a1e0720634bb0223be74d3c21a39e0e9 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Mon, 14 Jul 2025 17:43:15 -0700 Subject: [PATCH] Fix #293: improve error message for ObjectId collision fail --- release-notes/VERSION-2.x | 2 + .../jackson/annotation/ObjectIdGenerator.java | 19 ++++---- .../annotation/SimpleObjectIdResolver.java | 25 +++++++++-- .../SimpleObjectIdResolverTest.java | 44 +++++++++++++++++++ 4 files changed, 75 insertions(+), 15 deletions(-) create mode 100644 src/test/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolverTest.java diff --git a/release-notes/VERSION-2.x b/release-notes/VERSION-2.x index 3ebf1c93..a3df60d0 100644 --- a/release-notes/VERSION-2.x +++ b/release-notes/VERSION-2.x @@ -16,6 +16,8 @@ NOTE: Jackson 3.x components rely on 2.x annotations; there are no separate 2.20 (not yet released) +#293: Improve duplicate Id with different associated object error message + (requested by @moutyque) #294: Drop patch number from version for 2.20 and later (no more 2.20.0) #296: Drop Java 6 compatibility for 2.20 (Java 8 baseline) diff --git a/src/main/java/com/fasterxml/jackson/annotation/ObjectIdGenerator.java b/src/main/java/com/fasterxml/jackson/annotation/ObjectIdGenerator.java index d6263109..7e79fb1b 100644 --- a/src/main/java/com/fasterxml/jackson/annotation/ObjectIdGenerator.java +++ b/src/main/java/com/fasterxml/jackson/annotation/ObjectIdGenerator.java @@ -1,5 +1,7 @@ package com.fasterxml.jackson.annotation; +import java.util.Objects; + /** * Definition of API used for constructing Object Identifiers * (as annotated using {@link JsonIdentityInfo}). @@ -148,18 +150,13 @@ public final static class IdKey private final int hashCode; public IdKey(Class type, Class scope, Object key) { - if (key == null) { - throw new IllegalArgumentException("Can not construct IdKey for null key"); - } - this.type = type; + this.type = Objects.requireNonNull(type, "Type must not be null"); + // Scope can be null this.scope = scope; - this.key = key; + this.key = Objects.requireNonNull(key, "Key must not be null"); - int h = key.hashCode() + type.getName().hashCode(); - if (scope != null) { - h ^= scope.getName().hashCode(); - } - hashCode = h; + hashCode = Objects.hashCode(key) + Objects.hashCode(type.getName()) + ^ Objects.hashCode(scope); } @Override @@ -178,7 +175,7 @@ public boolean equals(Object o) @Override public String toString() { return String.format("[ObjectId: key=%s, type=%s, scope=%s]", key, - (type == null) ? "NONE" : type.getName(), + type.getName(), (scope == null) ? "NONE" : scope.getName()); } } diff --git a/src/main/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolver.java b/src/main/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolver.java index e887eb4d..bc038e95 100644 --- a/src/main/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolver.java +++ b/src/main/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolver.java @@ -11,7 +11,7 @@ * @author Pascal Gélinas */ public class SimpleObjectIdResolver implements ObjectIdResolver { - protected Map _items; + protected Map _items; public SimpleObjectIdResolver() { } @@ -19,7 +19,7 @@ public SimpleObjectIdResolver() { } public void bindItem(IdKey id, Object ob) { if (_items == null) { - _items = new HashMap(); + _items = new HashMap<>(); } else { Object old = _items.get(id); if (old != null) { @@ -27,13 +27,30 @@ public void bindItem(IdKey id, Object ob) if (old == ob) { return; } - throw new IllegalStateException("Already had POJO for id (" + id.key.getClass().getName() + ") [" + id - + "]"); + throw new IllegalStateException(String.format( +"Object Id conflict: Id %s already bound to an Object %s: attempt to re-bind to a different Object %s", + id.toString(), _desc(old), _desc(ob))); } } _items.put(id, ob); } + private String _desc(Object ob) { + if (ob == null) { + return "(null)"; + } + String desc; + if (ob instanceof String) { + desc = "\""+ob+"\""; + } else { + desc = ob.toString(); + if (desc.length() > 100) { + desc = desc.substring(0, 100) + "[... truncated]"; + } + } + return ("(type: `"+ob.getClass().getName()+"`, value: "+desc+")"); + } + @Override public Object resolveId(IdKey id) { return (_items == null) ? null : _items.get(id); diff --git a/src/test/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolverTest.java b/src/test/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolverTest.java new file mode 100644 index 00000000..895651c2 --- /dev/null +++ b/src/test/java/com/fasterxml/jackson/annotation/SimpleObjectIdResolverTest.java @@ -0,0 +1,44 @@ +package com.fasterxml.jackson.annotation; + +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.fail; + +public class SimpleObjectIdResolverTest +{ + @Test + public void testSimpleHandling() + { + // Let's create a simple ObjectIdResolver with 2 non-duplicate items + SimpleObjectIdResolver resolver = new SimpleObjectIdResolver(); + ObjectIdGenerator.IdKey key1 = new ObjectIdGenerator.IdKey(String.class, null, "key1"); + ObjectIdGenerator.IdKey key2 = new ObjectIdGenerator.IdKey(String.class, null, "key2"); + + // Bind 2 non-duplicate items, check they can be resolved + resolver.bindItem(key1, "value1"); + resolver.bindItem(key2, "value2"); + + assertEquals("value1", resolver.resolveId(key1)); + assertEquals("value2", resolver.resolveId(key2)); + assertEquals(2, resolver._items.size()); + + // And then verify that multiple bindings of the same key/bound value is ok + resolver.bindItem(key1, "value1"); + resolver.bindItem(key2, "value2"); + assertEquals(2, resolver._items.size()); + + // But that overriding is not + try { + resolver.bindItem(key1, "value3"); + fail("Should have thrown an exception for re-binding"); + } catch (IllegalStateException e) { + assertEquals( +"Object Id conflict: Id [ObjectId: key=key1, type=java.lang.String, scope=NONE]" ++" already bound to an Object (type: `java.lang.String`, value: \"value1\"):" ++" attempt to re-bind to a different Object (type: `java.lang.String`, value: \"value3\")", + e.getMessage()); + } + } + +}