Collection rendering with `partial:`/`collection:` shorthand reuses one message across iterations, leaking unset fields between elements

[jujustayfly]

Collection rendering with partial:/collection: shorthand reuses one message across iterations, leaking unset fields between elements

Affected version: pbbuilder 0.20.0 (and earlier — same shape since _render_collection_with_options was introduced).

Summary

When you render a repeated message field with the partial:, as:, collection: shorthand:

pb.items partial: "item", as: :item, collection: @items

PbbuilderTemplate#_render_collection_with_options allocates one sub-message and reuses it for every iteration of the partial. As a result, any field that is not actively (re)written by the partial on a given iteration carries over from the previous iteration.

In practice this affects:

• Repeated message fields written via pb.field collection do |element| ... end: those route through _append_repeated, which pushes onto the existing field rather than replacing it. With a shared message they accumulate across iterations.
• Conditionally-written fields: a partial branch like if/elsif that writes a field in some branches but not others leaks the previous element's value into elements that take the silent branch.

Manual iteration (pb.field coll.each do |element| ... end) is unaffected because _append_repeated allocates a fresh sub-message per element.

Root cause

lib/pbbuilder/template.rb _render_collection_with_options:

def _render_collection_with_options(field, collection, options)
  partial = options[:partial]

  options.reverse_merge! locals: options.except(:partial, :as, :collection, :cached)
  options.reverse_merge! ::PbbuilderTemplate.template_lookup_options

  options[:locals].merge!(pb: ::PbbuilderTemplate.new(@context, new_message_for(field)))
  options[:locals].merge!(pb_parent: self)
  options[:locals].merge!(field: field)

  ...

  CollectionRenderer
    .new(@context.lookup_context, options) { |&block| _scope(message[field.to_s], &block) }
    .render_collection_with_partial(collection, partial, @context, nil)
end

Note the third-from-last line: PbbuilderTemplate.new(@context, new_message_for(field)) is built once and merged into options[:locals][:pb]. ActionView::CollectionRenderer then renders the partial N times against that same pb and the same @message.

CollectionRenderer#build_rendered_template snapshots pb.attributes! (i.e. @message.to_h) per render, and build_rendered_collection rebuilds independent messages from those hashes via pb_parent.set!(field, templates.map(&:body)) — which makes the intent clear: each iteration is supposed to be an independent snapshot. But the snapshot reflects whatever @message looks like at that moment, including everything left behind by previous iterations.

Compare with manual iteration through _append_repeated (lib/pbbuilder.rb):

def _append_repeated(name, descriptor, collection, &block)
  ::Kernel.raise ::ArgumentError, "expected Enumerable" unless collection.respond_to?(:map)
  elements = collection.map do |element|
    message = _new_message_from_descriptor(descriptor)   # fresh per element
    _scope(message) { block.call(element) }
  end

  @message[name].push(*elements)
end

A fresh sub-message is allocated per element, so the same partial that leaks under collection-rendering does not leak when iterated this way.

Minimal repro

Schema:

message Item {
  string id = 1;
  repeated string tags = 2;
}
message Bag {
  repeated Item items = 1;
}

Partial _item.pbbuilder (writes tags only when there is something to write):

pb.id item.id
if item.tags.any?
  pb.tags item.tags
end

Top-level template:

pb.items partial: "item", as: :item, collection: [
  Item.new(id: "a", tags: ["x", "y"]),
  Item.new(id: "b", tags: [])
]

• Expected: bag.items[1].tags == [].
• Actual: bag.items[1].tags == ["x", "y"] (leaks from the previous element).

A simpler form of the same bug: any partial that writes a repeated message field via pb.field collection do |x| ... end will accumulate values across all iterations because the underlying _append_repeated push targets the shared @message.

Suggested fix

Allocate a fresh PbbuilderTemplate (and underlying message) per iteration, rather than once before the loop. The cleanest place to do that is inside Pbbuilder::CollectionRenderer so the per-element render starts from a clean message.

Sketch (in lib/pbbuilder/collection_renderer.rb):

class Pbbuilder
  class CollectionRenderer < ::ActionView::CollectionRenderer
    def initialize(lookup_context, options, &scope)
      super(lookup_context, options)
      @scope = scope
    end

    private

    def build_rendered_template(content, template, layout = nil)
      # Reset to a fresh sub-message before snapshotting so this iteration
      # cannot inherit fields from the previous one.
      pb.instance_variable_set(:@message, pb_parent.new_message_for(field))
      super(content || pb.attributes!, template)
    end

    # ... (build_rendered_collection / pb / pb_parent / field unchanged)
  end
end

Or — arguably cleaner — replace options[:locals][:pb] for each element so @message doesn't have to be poked directly. Either way the contract is "each partial render starts with a fresh message".

A test mirroring the conditional-repeated-field shape from the repro should be added to the spec suite to lock this down.

Workaround

Until this is fixed upstream, replace the partial:/collection: shorthand with manual iteration so each element renders into a fresh sub-message:

pb.items @items.each do |item|
  pb.partial! "item", item: item
end