Make AggregateRoot#unpublished_events public

[paneq]

Because it is necessary for testing and that's the public interface for
exposing generated domain events.

[mlomnicki]

I don't like it but I don't have a better idea so +1 for this

[andrzejkrzywda]

Would it be safer to return a clone of this list for the public accessor? I'm worried that debugging of situations when someone mutates this object will be difficult.

[andrzejsliwa]

yes, agree... I would keep original method private. Maybe returning iterator function will be better and much safer. And will not require always calling clone even for internal usage.

[andrzejsliwa]

for testing you can always use metaprograming:

require 'rspec/expectations'
require_relative 'utils'

RSpec::Matchers.define :publish do |expected_events|
  match do |aggregate|
    @stripped_actual   = as_string(aggregate.__send__("unpublished_events"))
    @stripped_expected = as_string(expected_events)
    expect(@stripped_actual).to eq @stripped_expected
  end

  failure_message do |aggregate|
    message = "Expected aggregate:\n\n#{aggregate.inspect}\n\nto " \
              "publish events:\n\n#{@stripped_actual}\n\nwould match:\n\n#{@stripped_expected}"
    message += "\n\nDiff: #{differ.diff_as_string(@stripped_actual, @stripped_expected)}"
    message
  end

  def differ
    RSpec::Support::Differ.new(
      color: RSpec::Matchers.configuration.color?
    )
  end
end

Its not perfect but I'm fine with using little bit of "magic". Exposing this method to the public will open way of overusing it or just misuse of aggregate. I know from past experiences that people even thinking about using aggregate root state as public and skip making read models, I don't think that giving here to much freedom is a best way to deal with testing problem

[paneq]

Would it be safer to return a clone of this list for the public accessor?

It would

returning iterator function will be better and much safer

lovely idea

def unpublished_events
  @unpublished_events ||= []
  @unpublished_events.each
end

for testing you can always use metaprograming:

that sounds like a bad design to me. If the purpose of an aggregate is to protect rules and generate events then those events are public. Public in a sense that someone needs to consume them and publish.

Unfortunately those safe options from production point of view does not solve the original issue that much. If you use unpublished_events for testing and you use commands to set up the aggregate state you need to clean unpublished_events or remember where new events were added.

not possible on cloned collection or iterator

specify "product supplied" do
  product.register(store_id: 1, sku: "BOOK")
  product.unpublished_events.clear
  product.supply(30)
  expect(product).to have_applied(ProductSupplied).only

not convinient

specify "product supplied" do
  product.register(store_id: 1, sku: "BOOK")
  size = product.unpublished_events.size
  product.supply(30)
  expect(product.unpublished_events[size..-1]).to match([an_event(ProductSupplied)])

workaround

I use the solution we had for #109 right now.

specify "product supplied" do
  product.register(store_id: 1, sku: "BOOK")
  events = product.supply(30)
  expect(events).to match([an_event(ProductSupplied)])

therefore fixing this is not that important for me.

However it seems that @andrzejsliwa is using aggregate.__send__("unpublished_events") and rails_event_store-rspec as well. I find it suboptimal.

Perhaps

def unpublished_events
  @unpublished_events ||= []
  @unpublished_events.each
end

is good enough to be safe and support those usecases. I would go for that.

[andrzejsliwa]

def unpublished_events
  @unpublished_events ||= []
  @unpublished_events.each
end

is good enough to be safe and support those usecases. I would go for that.

👍