From 665fff95ceec30edeaadb194dab98e426e5bed8c Mon Sep 17 00:00:00 2001 From: allockicmoi Date: Thu, 7 May 2026 14:55:08 +0100 Subject: [PATCH] Fix collection partial leaking fields between iterations (#64) `PbbuilderTemplate#_render_collection_with_options` allocated a single sub-message and reused it across every iteration of the partial, so any field not actively rewritten on a given iteration carried over from the previous one. In particular: - Repeated message fields written via `pb.field collection do |x| ... end` accumulated across iterations because `_append_repeated` pushes onto the shared message. - Conditionally-written fields leaked the previous element's value into elements that took the silent branch. Reset the working message on the shared `pb` between iterations inside `Pbbuilder::CollectionRenderer#build_rendered_template`, so each render starts with a clean sub-message of the right type. This brings the `partial:`/`collection:` shorthand to parity with the per-element allocation already done by manual iteration through `_append_repeated`. Adds a `_reset_message` helper on `Pbbuilder` (since the class inherits from `BasicObject` and doesn't expose `instance_variable_set`) plus two regression tests covering both leak shapes. Co-authored-by: Cursor --- CHANGELOG.md | 1 + lib/pbbuilder.rb | 7 ++++ lib/pbbuilder/collection_renderer.rb | 7 +++- test/pbbuilder_template_test.rb | 59 ++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c469c7..c2c2ca8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ ## Unreleased +- Fix `partial:`/`collection:` shorthand reusing one sub-message across iterations, which leaked unset fields and accumulated repeated message blocks between elements (#64) - Remove official support for Rails 7.0 and Rails 6.x, add Appraisal config for Rails 8.x - Bump actions/checkout from 3 to 4 (#40) - Drop support for rails v6.* versions (#60) diff --git a/lib/pbbuilder.rb b/lib/pbbuilder.rb index 5b76670..46f7a69 100644 --- a/lib/pbbuilder.rb +++ b/lib/pbbuilder.rb @@ -197,6 +197,13 @@ def new_message_for(field) _new_message_from_descriptor(descriptor) end + # Replaces the underlying message. Used by Pbbuilder::CollectionRenderer to + # give each iteration of a `partial:`/`collection:` render a clean message + # so fields cannot leak between elements. + def _reset_message(message) + @message = message + end + private # Lookup the field name (or 'attribute' name, for ex "best_friend") on the Google descriptor (Google::Protobuf::Descriptor) of our diff --git a/lib/pbbuilder/collection_renderer.rb b/lib/pbbuilder/collection_renderer.rb index 9a3535c..00133c4 100644 --- a/lib/pbbuilder/collection_renderer.rb +++ b/lib/pbbuilder/collection_renderer.rb @@ -12,7 +12,12 @@ def initialize(lookup_context, options, &scope) private def build_rendered_template(content, template, layout = nil) - super(content || pb.attributes!, template) + body = content || pb.attributes! + # Reset the working message so the next iteration starts with a clean + # sub-message and cannot inherit fields written or pushed by this one. + # See https://github.com/cheddar-me/pbbuilder/issues/64. + pb._reset_message(pb_parent.new_message_for(field)) + super(body, template) end def build_rendered_collection(templates, _spacer) diff --git a/test/pbbuilder_template_test.rb b/test/pbbuilder_template_test.rb index 7e17d35..c882e41 100644 --- a/test/pbbuilder_template_test.rb +++ b/test/pbbuilder_template_test.rb @@ -21,11 +21,25 @@ class PbbuilderTemplateTest < ActiveSupport::TestCase pb.url_3x asset.url PBBUILDER + TAGGABLE_PARTIAL = <<-PBBUILDER + pb.name taggable.name + pb.tags taggable.tags if taggable.tags.any? + PBBUILDER + + FRIEND_HOLDER_PARTIAL = <<-PBBUILDER + pb.name holder.name + pb.friends holder.friends do |f| + pb.name f.name + end + PBBUILDER + PARTIALS = { "_partial.pb.pbbuilder" => "pb.name name", "_person.pb.pbbuilder" => PERSON_PARTIAL, "racers/_racer.pb.pbbuilder" => RACER_PARTIAL, "_asset.pb.pbbuilder" => ASSET_PARTIAL, + "_taggable.pb.pbbuilder" => TAGGABLE_PARTIAL, + "_friend_holder.pb.pbbuilder" => FRIEND_HOLDER_PARTIAL, # Ensure we find only Pbbuilder partials from within Pbbuilder templates. "_person.html.erb" => "Hello world!" @@ -89,6 +103,51 @@ class PbbuilderTemplateTest < ActiveSupport::TestCase assert_equal 2, result.friends.count end + # Regression test for https://github.com/cheddar-me/pbbuilder/issues/64. + # A partial that conditionally writes a repeated scalar field used to leak + # the previous element's value into elements that took the silent branch + # because the underlying message was reused across iterations. + test "collection partial does not leak conditional repeated scalar fields between iterations" do + template = <<-PBBUILDER + taggable_struct = Struct.new(:name, :tags) + items = [ + taggable_struct.new("alice", ["x", "y"]), + taggable_struct.new("bob", []), + ] + pb.friends partial: "taggable", as: :taggable, collection: items + PBBUILDER + result = render(template) + + assert_equal 2, result.friends.count + assert_equal "alice", result.friends[0].name + assert_equal ["x", "y"], result.friends[0].tags.to_a + assert_equal "bob", result.friends[1].name + assert_equal [], result.friends[1].tags.to_a + end + + # Regression test for https://github.com/cheddar-me/pbbuilder/issues/64. + # `pb.field collection do |x| ... end` inside a collection partial used to + # accumulate elements across iterations because _append_repeated pushed onto + # the shared message's repeated field. + test "collection partial does not accumulate repeated message blocks between iterations" do + template = <<-PBBUILDER + holder_struct = Struct.new(:name, :friends) + friend_struct = Struct.new(:name) + items = [ + holder_struct.new("alice", [friend_struct.new("a1"), friend_struct.new("a2")]), + holder_struct.new("bob", []), + ] + pb.friends partial: "friend_holder", as: :holder, collection: items + PBBUILDER + result = render(template) + + assert_equal 2, result.friends.count + assert_equal "alice", result.friends[0].name + assert_equal ["a1", "a2"], result.friends[0].friends.map(&:name) + assert_equal "bob", result.friends[1].name + assert_equal 0, result.friends[1].friends.count + end + test "partial by name with top-level locals" do result = render('pb.partial! "partial", name: "hello"') assert_equal "hello", result.name