Forward Object Id references inside Collection properties leak Builder instance (follow-up to #1496)

[cowtowncoder]

Follow-up to #1496.

#1496 enabled Object Ids with Builder-based deserialization for the common cases and added clear error reporting (InvalidDefinitionException) for forward Object Id references bound to scalar properties, where the reference would otherwise be silently resolved against the discarded Builder instance.

One case remains broken: forward Object Id references that appear as items of a Collection-typed property. The collection-item path does not go through ObjectIdReferenceProperty.deserializeSetAndReturn, so DeserializationContext.addPendingForwardRef is never called and the guard in BuilderBasedDeserializer.finishBuild (hasPendingForwardRefsFor) does not trip. As a result:

• No error is reported.
• The unresolved slot ends up holding the Builder instance instead of the built target object after resolution.

Reproduction

See src/test/java/tools/jackson/databind/tofix/ObjectIdWithBuilder1496Test.java (forwardReferenceInCollection, currently @JacksonTestFailureExpected):

{"entities":[
  {"id":1,"refs":[2]},
  {"id":2,"ref":1,"refs":[]}
]}

Entity uses @JsonIdentityInfo + @JsonDeserialize(builder = EntityBuilder.class). After deserialization, entities[0].refs[0] is an EntityBuilder rather than the built Entity with id 2.

expected: Entity@... but was: EntityBuilder@...

Suggested fix direction

Two options, both requiring changes beyond the scope of #1496:

1. Plumb collection-item forward references through addPendingForwardRef so the existing BuilderBasedDeserializer.finishBuild guard catches them and emits the same InvalidDefinitionException ("Cannot resolve forward Object Id references for Builder-based type …").
2. Actually support the case: re-bind collection-item references from the builder to the built object when updateObjectId is called in finishBuild. This would likely require the collection Referring implementation to late-bind via the ReadableObjectId entry after updateObjectId rewrites the bound item.

Option 1 matches the existing fail-fast contract for the scalar case and is the minimum acceptable fix. Option 2 is the full solution.

Version

3.x (observed on current 3.x branch after #1496 / commit 476f3d9f5).

[kyooosukedn]

Traced through the code to nail down exactly where the collection item gets the builder. Here is the execution order for the failing test case.

JSON input:

{"entities":[
  {"id":1,"refs":[2]},
  {"id":2,"ref":1,"refs":[]}
]}

Step by step:

1. Entity 1 builder is deserialized. id:1 is set. ref is null. refs: [2] triggers CollectionDeserializer._deserializeWithObjectId. The 2 is a forward Object Id reference (Entity 2 hasn't been seen yet). UnresolvedForwardReference is caught at line 515, a CollectionReferring is created and appended to the ReadableObjectId for id=2 via appendReferring.

2. Entity 1 finishBuild runs. No pending forward refs for this builder (the pending ref is on Entity 2's ReadableObjectId, not Entity 1's). _objectIdReader != null and ctxt.hasPendingForwardRefsFor(builder1) is false, so no exception. The builder builds entity1. updateObjectId(builder1, entity1) rebinds the OID entry for id=1 from builder1 to entity1. Entity 1's refs list still has the unresolved slot from the accumulator.

3. Entity 2 builder is deserialized. id:2 is set. ref: 1 is resolved — ObjectIdReferenceProperty.deserializeSetAndReturn looks up id=1, gets entity1 (already rebound by step 2), sets it on the builder. refs: [] is empty, no issue.

4. Entity 2 bindItem fires (ReadableObjectId.bindItem(ctxt, builder2) at line 63). This resolves all pending Referring instances for id=2. The CollectionReferring from step 1 is still in the list. handleResolvedForwardReference(ctxt, id=2, value=builder2) is called. The CollectionReferringAccumulator.resolveForwardReference puts builder2 into Entity 1's refs list. This is the bug — builder2 lands in the collection, not entity2.

5. Entity 2 finishBuild runs. updateObjectId(builder2, entity2) calls tryReplaceBoundItem, which updates the _item field in the ReadableObjectId to entity2. But _referringProperties was already nulled out in bindItem (line 70: _referringProperties = null), and the collection already holds builder2. The rebind does not propagate to already-resolved collection items.

The gap: ObjectIdReferenceProperty.deserializeSetAndReturn calls ctxt.addPendingForwardRef(instance) when it catches a forward ref on a scalar property. This is what makes the scalar case fail-fast in finishBuild. The collection path (CollectionDeserializer._deserializeWithObjectId, line 515) catches UnresolvedForwardReference too, but never calls addPendingForwardRef. So the builder guard in finishBuild never trips for collection-item forward refs.

Option 1 fix (fail-fast, matches scalar behavior): Call ctxt.addPendingForwardRef(...) in the collection forward ref handler too. The tricky part is what to pass as the container — it should be the builder instance that owns the collection, not the collection itself. The CollectionDeserializer doesn't currently have a reference to the builder. This would need plumbing.

Option 2 fix (actually resolve correctly): Make updateObjectId / tryReplaceBoundItem propagate the rebind to previously-resolved collection items. This is harder — _referringProperties is already cleared by the time updateObjectId runs, so there's nothing to re-notify. You'd need a separate mechanism: either keep the Referring instances alive past bindItem, or have the CollectionReferringAccumulator register for a post-build callback.

I haven't submitted a PR — this needs a design decision on Option 1 vs Option 2. Happy to implement whichever direction you prefer.

[cowtowncoder]

Hmmh. I don't know -- neither path sounds particularly appetizing. Which genAI tool did you use for the analysis @kyooosukedn ?

[kyooosukedn]

Hi @cowtowncoder, my apologies for my agent :) I used Claude to help trace through the code paths.
Honestly, I'm still trying to understand the problem.

So the core issue is that ReadableObjectId.bindItem() resolves forward refs with the builder before finishBuild() runs, and by the time updateObjectId() fires, _referringProperties is already nulled out, so the collection holds a stale reference. This same problem would affect ArrayReferring, MapReferring, and AnySetterReferring too, not just collections. The rebind callback approach seems clean because it's opt-in. Scalar PropertyReferring doesn't need it since the built entity captures values through its constructor. One thing I'm not sure about: what happens with nested builders? If a collection item itself is a builder type with its own forward refs, the rebind chain could get interesting. One thing I'm not sure about: Referring is a public abstract class. Adding handleItemRebind() as a default no-op method is a behavioral change for anyone who sub-classed it; they'd silently ignore rebinds. Should this be abstract instead, forcing all implementations to think about it? Or is the no-op safe because custom Referring implementations presumably aren't used with builder-based types?

[cowtowncoder]

@kyooosukedn Thank you for going through this, picking up an important if challenging problem.

Ok so option (2) does sound cleaner. I wonder if it'd be possible to clear "properly resolved" referrees eagerly still, and only leave builder ones (but I may misunderstand nature of these refs).
Nested builders case is interesting, but perhaps to be worried about after resolving non-nested case (although... with GenAI why not verify upfront?).

On abstract vs default: I'd lean towards default no-op; avoids hard-breakage in (unlikely) case of something outside databind implementes Referring. Could hide problems, true, but I think it's more likely default is no worse than behavior before this fix.

Does this make sense?

[kyooosukedn]

@cowtowncoder Yes, so my thoughts:
On eager clearing, I think I follow the idea, having two Referring implementations where non-builder ones resolve immediately and builder ones stay pending until finishBuild. Will need to dig into it more to make sure I'm not missing something, but directionally it sounds right. I'll just write the test and report back on nested builders case.
On default vs abstract I agreed with default no-op.