feat: return nil aggregate state when the aggregate does not exist

[fahchen]

I think it is more appropriate to return nil aggregate state when the aggregate does not exist. We can also build the initial aggregate state with struct(aggregate_module) easily.

[jvantuyl]

I agree. It's likely to be more performant, too (if only slightly).

That said, I think this wouldn't be backwards-incompatible. Anyone that previously detects a missing aggregate correctly will have their code immediately start seeing nil values that it never saw before. So maybe it's only appropriate for a major release and should be well documented. Or, alternatively, it could maybe be feature-flagged with a compile-time config option.

[yordis]

The intent here matters a lot, and what do you mean by "the aggregate does not exist"

[fahchen]

The intent here matters a lot, and what do you mean by "the aggregate does not exist"

If an aggregate cannot be reconstructed from events, can we assume that it does not exist?

[jvantuyl]

I understand it to mean: "Asking for an aggregate with an ID that doesn't correspond to any known aggregate."

The intuition here is that returning effectively an empty struct makes it appear that there's an aggregate where there is not really one. Returning nil for aggregate_state and 0 for aggregate_version makes a lot of sense.

The alternative is that detecting a bad ID is kind of non-intuitive and, if you don't check carefully, you may also start using what is effectively an uninitialized aggregate unknowingly.

[yordis]

They are situations where you do not set the aggregate ID ever in the aggregate itself because you are leveraging the stream as a consistency boundary because they are many ways to "initiate" that stream to the point that a "not found" would never happen, neither "uninitialized" ...

You must be extremely careful with the assumption of nil.

Aggregate has its own "Initiate State" that people must respect, and in some cases, that "initiate/default struct state" IS the correct state.

In other implementations that don't have the behavior of Elixir Structs with default values, people would have an fn initiate_state() -> state function, and whatever is returned there, you must respect it.

In other words, I'm afraid I have to disagree with the assumption made here. The aggregate default/initial state must be respected, and it is not about having an event version.

defmodule MyAggregate do
  defstruct [status: :PENDING]

  def execute(aggregate, command) do
    # does not matter if the event count is 0 or not; the value, therefore, the state of the
    # aggregate must be `%MyAggregate{status: :PENDING}` NOT `nil`, including from outside
    # the command handler
    %MyEvent{status: aggregate.status}
  end
end

---

A second argument could be about the "Initial State function," today's Elixir __struct__/0 function behavior, and an alternative. But that is an entirely different argument.

[fahchen]

The changes impact solely the aggregate_state function and do not influence the internal logic, such as your own aggregates.

[yordis]

As I said in the comment inside the code snippet (I should move outside), regardless if you are speaking about the Command Handler context (inside the execute/2), still important.

Because if you use nil, you will need to read source code files in production/environment to know the value of status. The worst situation is if two deployments have different values because the business requirements changed and the status value is different.

The changes make assumptions that only apply to a given perspective to see the situation. Aside from being a breaking change, it would only work for some situations.

Whatever situation made prompted you to ask for such functionality probably could change and add enough "Domain level" information that would allow you to know better the state of your aggregate.

You probably have a really good situation on hand, I am extremely curious to know what prompted you to do these changes. Do you mind sharing here?

[fahchen]

Because if you use nil, you will need to read source code files in production/environment to know the value of status. The worst situation is if two deployments have different values because the business requirements changed and the status value is different.

We can construct the initial state via stuct(__MODULE__) if necessary, this is how the aggregate builder does now.

commanded/lib/commanded/aggregates/aggregate_state_builder.ex

Line 32 in e20fddd

%Aggregate{state | aggregate_version: 0, aggregate_state: struct(aggregate_module)}

You said maybe we need a function to construct the initial state. I think it depends. For now the initial state is determined at compile time, after adding a construct function, the initial state may be determined at runtime. I don’t think it’s a good idea that the construct function generates different initial states at, because this may make the initial state undetermined, and may impact the eventual consistency, may make commands handlers more complicated.

[yordis]

I shared my concern about these changes and I am strongly against them, to begin with, it would be a breaking change since such public function is leverage for different use cases, my personal situation, a Phoenix Dashboard for Commanded and production debugging tooling.

---

We can construct the initial state via stuct(MODULE) if necessary, this is how the aggregate builder does now.

That is exactly what I do not want to be doing in production!

Such technicality should be abstracted away, reasons why some peeps (like me) use aggregate_state function, it is not just about that!

We are going in a circle by now, I asked you to share your concrete use case rather than bikeshed over code for the sake of code, everything makes sense out of context. I am not saying you are wrong completely, just that these changes are missing out on a lot of contexts and making assumptions.

In the end, It Depends ™️ so I don't think it will be a productive conversation.

[fahchen]

@yordis Let's separate things apart, I found that problems always seem to be discussed together.

1. Why is it necessary to differentiate the initial state using the nil value

Determining whether a state(it is not the Commanded.Aggregates.Aggregate gen_server state, it is what the function returns) is initial can be hard when using the aggregate_state function. (I’ll call Commanded.Aggregates.Aggregate gen_server state as gen_server state, what the function returns as aggregate state later.)

I’ll quota your code to explain.

defmodule MyAggregate do
  defstruct [status: :PENDING]

  def execute(aggregate, command) do
    %MyEvent{status: aggregate.status}
  end
end

The aggregate state has no obvious characteristics to distinguish whether it is in its initial state. Even if we read source code files, We can’t determinate whether the aggregate state (eg: %MyAggregate{status: :PENDING}) is initial, for the status could be changed back to :PENDING after some events applied.

Maybe, you could add some id-ish(please tell aggregate_uuid of Commanded.Aggregates.Aggregate apart) to the aggregate state, but there isn't a generic way to handle this problem.

We can’t get the gen_server state via api, however, I do think it’s not a good idea to expose that out directly(We can discuss in other parts, not in this part).

So base these, a generic way to distinguish whether an aggregate state is in its initial state is necessary.

2. How we make a compatible change

If you agree with the above part, we can discuss this part further.
For now, I don’t have any idea except this PR, maybe we can provide another function or function parameter to do that. But, let’s reach an agreement on the above problem firstly.

[yordis]

What do you intend to do in your business?

Like, remove the technicality of the situation for a moment. That is what I am trying to get out of you to comprehend and help you better altogether.

I understand your problem with the existing implementation and the what and how you are trying to change, but I still don't know the why behind it because it could be an XY problem situation here. Please read https://xyproblem.info/ if you need to become more familiar with it.

Why do you need to determine whether the aggregate state is the initial state? ESPECIALLY if you are not building dev tools and it is a domain need (concerning if so), more often than not, people think of the problem in a sub-optimal way.

[fahchen]

Here is my situation:
My service could handle requests from outside, from users or other systems, which are handled by aggregate_state function. I have to tell them the aggregate’s existence accurately, for they don’t have context.

FYI: Why this feature introduced, and there is a case that shares with mine, see #336

[jvantuyl]

@fahchen For what it's worth, this is why I tend to have my enums default to an UNKNOWN value. It helps distinguish the zero value from a fresh but initialized value. I picked this up in Go, but it works for this here. Not sure if that works for your use case or not, but it's not a terrible approach from my experience.

[jvantuyl]

@yordis Maybe I'm just misunderstanding the use-case. How exactly is the aggregate_state/3 meant to be used?

[fahchen]

@fahchen For what it's worth, this is why I tend to have my enums default to an UNKNOWN value. It helps distinguish the zero value from a fresh but initialized value. I picked this up in Go, but it works for this here. Not sure if that works for your use case or not, but it's not a terrible approach from my experience.

@jvantuyl I agree with you that we can have an unknown status as the initial flag. However, if we could identify the initial state, even without altering any bytes of our business code, that would be a just enough approach.

[jvantuyl]

@jvantuyl I agree with you that we can have an unknown status as the initial flag. However, if we could identify the initial state, even without altering any bytes of our business code, that would be a just enough approach.

I would support this. Semantically, nil is well understood to mean "doesn't exist". Making the user craft their own version of this for every aggregate doesn't sit well with me either. But... yeah... I suggest the workaround mostly for when other people stumble across this and need a solution.

[drteeth]

@yordis Maybe I'm just misunderstanding the use-case. How exactly is the aggregate_state/3 meant to be used?

I think a lot of the discussion so far is a little bit unclear because of the focus on aggregate_state/3. I think @yordis (and me while I was first reading) missed that we were even talking about aggregate_state/3. There is a focus here on aggregate_state/3 and I think @yordis is right to question what you are trying to do and why aggregate_state/3 seems to be the preferred tool.

To me, it's not something I would reach for unless I had run out of options elsewhere. See @jwilger's comment in #336. You've described your use case and I'm not clear why an event handler/read model wouldn't suit your needs. It would be more idiomatic IMO and you could customize to your heart's content. Most of all you'd be crafting a domain specific solution with existing mechanics instead of trying to bend the mechanics of aggregate_state/3 to your domain problem. Are you sure you can't handle this with a read model? If you elaborate more on your use case, I'm sure someone here (or in the commanded slack channel) can help find you such a solution.

As for the breaking change, I don't see how aggregate_state/3 changing it's signature would be a good thing. A new function gets around that but makes the choice of when to use one over the other unclear.

Finally, while nil/null is widely used in the industry to indicate the absence of a value, it is considered a anti-pattern by many. I would rather see an ok-error tuple if we have to have a change here.

For fun, see null's "creator" regretting its introduction in 1965:
The Billion Dollar Mistake