GH-1137 avoid populating version second time during update and insert

[mipo256]

Addressing #1137 issue. Jens @schauder, can you, please, review the changes.

[schauder]

Thanks for the PR.
Most of it looks really good. There is only one major concern: We can't remove public classes, methods, or constructors without deprecating them first, except for major versions. So we should make those changes that the original signatures stay available but are deprecated.

[mipo256]

Thanks for the PR. Most of it looks really good. There is only one major concern: We can't remove public classes, methods, or constructors without deprecating them first, except for major versions. So we should make those changes that the original signatures stay available but are deprecated.

Yes, you are right, I have forgotten about it :) I will fix this shortly, thank you very much for reviewing PR, appretiate it)

[mipo256]

As requested, I have deprecated the api that will not be used anymore if we apply this change :) Jens @schauder, can you, please, take a look?)

[schauder]

Why do you think the behaviour tested by this class should change?

[mipo256]

That might take some time to explain. Let me try :)

Previously we utilized JdbcAggregateChangeExecutionContext#populateIdsIfNecessary to set Ids to aggregates and other stuff. This method used to return null, if the entity after execution of DML have not been changed (simple example of how entity can be changed - there could be an auto generated identified provided by DB). And then after that we assigned the version to the entity. The essense of the problem were the follisng - both populateIdsIfNecessary method, or AggregateChange#getEntity method return entity without any version, even if we assign it previously.

Now, JdbcAggregateChangeExecutionContext can store in results map InsertExecutionResult object, which contains an entity with version - that is necessary, because we want to avoid redundant version population, but here is the problem - method JdbcAggregateChangeExecutionContext#populateIdsIfNecessary (I have given in my PR the more human readable, at least from my perspective, name to the method - getRootWithPopulatedFieldsFromExecutionResult) should not return null anymore, even if entity did not changed, becuase only JdbcAggregateChangeExecutionContext knows the version that was assigned to an entity. If we would return null, then we will loose this version information - just to remind, AggregateChange#getEntity does not know anything about version assigned to given entity, so if we will allow to return null, we will fallback to initaill issue.

This test case relied on fact, that AggregateChangeExecutor#execute will return an entity that is stored inside aggregateChange - wich is an entity from mock , the third one, but now it is no longer the truth. Now, the inserted entity will always be taken from InsertExecutionResult, which is the second one. Thus this test needs to be a little bit changed

I really did my best to explain it :) @schauder Jens, in case of any further questions - do reach me out, I will clarify everything :)

[schauder]

The problem with this is the test is a very valid test for the working of callbacks.
We need to continue to work with the third value returned by the mock in the original version of the test, because it is a value a user might return from a callback.

[mipo256]

Jens @schauder, quite, I got it. So lets segregate 2 things - the user would expect, that the return value of the tamplate#save() method will be the same as the entity, returned from AfterSaveCallback, right? I agree,and I have fixed this, thanks! But becuase of the reasons I have explained upwards, AfterSaveCallback will be populated with the second, not third entity. That thing will remain, right @schauder? :)

[schauder]

AfterSaveCallback will be populated with the second, not third entity.

No, that is not acceptable. The callback have to behave like a pipeline. What comes out of one needs to go into the next. The version update should be a part of that pipeline, so it must get its initial value from one callback and its result must go into the next callback.

[mipo256]

Ok, I got it) So I have fixed this, Jens @schauder, can you please re-review the changes?)

[schauder]

Please rebase this on the current main branch and squash it into a single commit.

[ctailor2]

The proposed changes to JdbcAggregateChangeExecutionContext break the method contract and will require updates ensure that this is not the case. Please see the line comments for further details.

[ctailor2]

As this method is not public I believe it could be removed rather than deprecated. Is my thinking correct @schauder ? If so, that would apply to the private methods related to the version field as well.

[mipo256]

I agree here, that make sence, let's await for the @schauder assesment

[schauder]

Correct, we need to maintain backward compatibility for code created by our users.

The most obvious is: we can't change/remove public methods. We also can't add methods to an interface without default implementation, or change protected methods which might be overwritten or called by classes created by users.

Everything that isn't directly reachable by the users is up for grabs.

[mipo256]

Ok, got it, thanks, so I will remove the non-public api, thanks guys!

[ctailor2]

Setting this variable for all InsertExecutionResult instead of only those whose action is instanceof DbAction.InsertRoot will cause some unexpected behavior for inserts on aggregates whose root is mutable but contains immutable references. In this scenario the return value of this method call would be a non-root entity rather than the aggregate root, which would break the method contract.

Add the following test to JdbcAggregateChangeExecutorContextImmutableUnitTests to see an example of this behavior.

@Test
public void idGenerationOfChild_withMutableRoot() {

	Content content = new Content();

	when(accessStrategy.insert(any(DummyEntityMutableNoVersion.class), eq(DummyEntityMutableNoVersion.class), eq(Identifier.empty()))).thenReturn(23L);
	when(accessStrategy.insert(any(Content.class), eq(Content.class), eq(createBackRef()))).thenReturn(24L);

	DbAction.InsertRoot<DummyEntityMutableNoVersion> rootInsert = new DbAction.InsertRoot<>(new DummyEntityMutableNoVersion());
	executionContext.executeInsertRoot(rootInsert);
	executionContext.executeInsert(createInsert(rootInsert, "content", content, null));

        // This call results in a ClassCastException as it returns an instance of Content rather than DummyEntityMutableNoVersion
	DummyEntityMutableNoVersion newRoot = executionContext.getRootWithPopulatedFieldsFromExecutionResult();

	assertThat(newRoot).isNotNull();
	assertThat(newRoot.id).isEqualTo(23L);

	assertThat(newRoot.content.id).isEqualTo(24L);
}

@Data
@AllArgsConstructor
@NoArgsConstructor
private static class DummyEntityMutableNoVersion {
	@Id private Long id;

	private Content content;
}

[mipo256]

Good catch, let me double check it, thank you very much for your suggestion.

[ctailor2]

Over the course of exploring a possible design for GH-537 where AggregateChange represents the change plan via DbAction for multiple entities and JdbcAggregateChangeExecutionContext is capable of being used for multiple roots, I happened to explore a refactor that deals with this same concern but uses a different approach.

I wasn't previously aware of the issue that this PR closes, but since my changes also seem to address the extraneous constructor calls, I figured I would cross reference it here for possible consideration.

[schauder]

I'd like to go forward with the PR by @ctailor2
The result is easier to understand at least for me.

@mikhail2048 I'm sorry that you put so much work into this before I turn around and go with a different PR. Your work on this, on previous and hopefully future PRs is still very much appreciated.

I leave this open for now until I can actually merge the other PR.

[mipo256]

A pity, but OK, at the end of the day the goal is to make framework better :) Thank you fellows @schauder @ctailor2 !)