Fix how the `Migration`s with custom state update are applied #1298

dmitrykuzmin · 2020-09-02T21:10:54Z

This PR fixes #1297.

The root cause of the bug is a discrepancy between entity and underlying transaction versions in Migration when updating the entity state.

This PR thus adds the missing setVersion(...) call.

It also applies some refactoring to the Migration class. Its current logic is confusing, with entity updates being propagated partially in-place, partially on transaction commit. This leads to strange code paths and observed values when debugging.

After the refactoring, all entity changes will be propagated on transaction commit at the very end of Migration.applyTo(...).

Also, refactor the code so the entity changes are transaction-based and propagated to the entity on transaction commit, instead of being applied in-place. This should make the migration logic as well as the values observed when debugging clearer.

…into fix-migrations

Removing this workaround leads to heavily verbose setup for some of the test cases, to the point where it makes it harder to understand the original meaning of the test. To address the problem, the concept of having `repository.create(ID)` which returns an invalid entity, which then cannot be loaded, should be revisited as a whole.

codecov · 2020-09-02T21:25:45Z

Codecov Report

Merging #1298 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master    #1298      +/-   ##
============================================
+ Coverage     91.04%   91.08%   +0.04%     
- Complexity     4745     4747       +2     
============================================
  Files           608      608              
  Lines         15088    15091       +3     
  Branches        854      854              
============================================
+ Hits          13737    13746       +9     
+ Misses         1083     1080       -3     
+ Partials        268      265       -3

dmitrykuzmin · 2020-09-02T21:57:56Z

@armiol, @yuri-sergiichuk PTAL.

yuri-sergiichuk

@dmitrykuzmin PTAL at the comments below.

yuri-sergiichuk · 2020-09-03T06:11:31Z

server/src/main/java/io/spine/server/entity/Migration.java

@@ -238,6 +239,7 @@ private void warnOnNoSystemEventsPosted() {
        private boolean physicallyRemoveRecord;

        private final E entity;
+        private @MonotonicNonNull Transaction<I, E, S, ?> tx;


Could you please reorder fields so that finals are grouped together?

Also, it looks like the tx should be marked as @LazyInit as well, right?

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

yuri-sergiichuk · 2020-09-03T08:47:27Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

+        assertThat(entityWithColumns.state()
+                                    .getIdString()).isEqualTo(id.toString());


I'd actually compare state and not the ID only.

server/src/test/java/io/spine/server/projection/given/SetTestProjectionName.java

server/src/test/java/io/spine/server/procman/given/repo/SetTestProcessName.java

yuri-sergiichuk · 2020-09-03T08:49:39Z

server/src/test/java/io/spine/server/procman/given/repo/SetTestProcessName.java

+public final class SetTestProcessName
+        extends ProcessManagerMigration<ProjectId, TestProcessManager, Project, Project.Builder> {
+
+    public static final String NEW_NAME = "Migrated project";


Maybe Migrated process? You have Migrated projection in the projection migration below.

yuri-sergiichuk · 2020-09-03T08:50:46Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

@@ -738,7 +815,8 @@ void deleteEntityViaMigration() {

        Optional<TestProcessManager> found = repository().find(id);
        assertThat(found.isPresent()).isTrue();


There's a whole set of Truth8 assertions that should allow having a bit nicer syntax for Optionals. Could you please check it out and consider using them instead?

server/src/main/java/io/spine/server/entity/Migration.java

yuri-sergiichuk · 2020-09-03T08:55:27Z

server/src/main/java/io/spine/server/entity/Migration.java

@@ -238,6 +239,7 @@ private void warnOnNoSystemEventsPosted() {
        private boolean physicallyRemoveRecord;


In the Operation docs it states that it's the operation done on an entity instance. In fact, we apply the migration to the tx().builder() now, right?

I guess it really worth adding some more details on how the migration operation is actually being done while you're in the context now.

…SetTestProjectionName.java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

…o/SetTestProcessName.java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

…into fix-migrations

dmitrykuzmin · 2020-09-03T12:32:05Z

@yuri-sergiichuk PTAL again.

yuri-sergiichuk

@dmitrykuzmin PTAL at the comments.

yuri-sergiichuk · 2020-09-03T13:13:36Z

server/src/main/java/io/spine/server/entity/Migration.java

+     * <p>The operation is performed in scope of an active {@link Transaction}.
+     *
+     * <p>All entity state and meta-data changes are propagated to the transaction and remain in
+     * pending state until a transaction {@linkplain Transaction#commit() commit} which is the


I guess it should be either a transaction is committed or a transaction commit is called

yuri-sergiichuk · 2020-09-03T14:31:57Z

server/src/main/java/io/spine/server/entity/Migration.java

-     * {@linkplain Operation operation}.
+     * Applies the migration {@linkplain Operation operation} to a given entity.
+     *
+     * @see Operation Migration.Operation for details


This see section looks redundant. You have the very same link as for operation. I'd better remove it.

server/src/main/java/io/spine/server/entity/Migration.java

yuri-sergiichuk · 2020-09-03T14:35:32Z

server/src/test/java/io/spine/server/bus/BusFilterTest.java

@@ -71,15 +72,15 @@ void passNullToleranceCheck() {
    void letPass() {
        BusFilter<CommandEnvelope> filter = new BusFilters.Accepting();
        Optional<Ack> ack = filter.filter(commandEnvelope);
-        assertThat(ack.isPresent()).isFalse();
+        Truth8.assertThat(ack).isEmpty();


Please import assertThat statically here and in the other places.

yuri-sergiichuk · 2020-09-03T14:39:48Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

@@ -675,7 +754,8 @@ void updateColumns() {

        // Check the column value is propagated to the entity state.
        TestProcessManager entityWithColumns = afterMigration.next();
-        assertThat(entityWithColumns.state().getIdString()).isEqualTo(id.toString());
+        assertThat(entityWithColumns.state()


Can we please have expected state here as well?

yuri-sergiichuk · 2020-09-03T14:41:03Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

@@ -723,8 +803,9 @@ void archiveEntityViaMigration() {
        repository().applyMigration(id, new MarkPmArchived<>());

        Optional<TestProcessManager> found = repository().find(id);
-        assertThat(found.isPresent()).isTrue();
-        assertThat(found.get().isArchived()).isTrue();
+        Truth8.assertThat(found).isPresent();


Please use static imports.

yuri-sergiichuk · 2020-09-03T14:41:58Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

@@ -773,6 +855,7 @@ private static TargetFilters targetFilters(QueryFilter first, QueryFilter... res
    }

    private static boolean hasId(TestProcessManager projection, ProjectId id) {
-        return projection.id().equals(id);
+        return projection.id()


Let's flip the params. You'll then be able to have them on the same line.

yuri-sergiichuk · 2020-09-03T14:42:31Z

server/src/test/java/io/spine/server/projection/ProjectionRepositoryTest.java

+        assertThat(results)
+                .comparingElementsUsing(idCorrespondence())
+                .containsExactly(id1, id2);


Can we test states here as well?

yuri-sergiichuk · 2020-09-03T14:42:40Z

server/src/test/java/io/spine/server/projection/ProjectionRepositoryTest.java

@@ -623,7 +699,7 @@ void archiveEntityViaMigration() {
        repository().applyMigration(id, new MarkProjectionArchived<>());

        Optional<TestProjection> found = repository().find(id);
-        assertThat(found.isPresent()).isTrue();
+        Truth8.assertThat(found).isPresent();


Please use static imports.

…java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

# Conflicts: # server/src/main/java/io/spine/server/entity/Migration.java

dmitrykuzmin · 2020-09-03T17:10:04Z

@yuri-sergiichuk PTAL again.

yuri-sergiichuk · 2020-09-03T18:01:29Z

server/src/test/java/io/spine/server/procman/ProcessManagerRepositoryTest.java

+        assertThat(results).hasSize(2);
+        assertThat(results)
+                .comparingElementsUsing(stateWithPropagatedName())
+                .containsExactly(pm1, pm2);


So the migration does nothing? We're comparing the results to the same PMs we've manually stored.

OK, now I see that inside the correspondence we actually manually change the state of the PMs to match the actual one. It looks a bit too complicated...

yuri-sergiichuk

@dmitrykuzmin in general LGTM.

I'd prefer to have a bit more straight-forward states comparison.

yuri-sergiichuk and others added 11 commits September 2, 2020 12:06

Never allow empty IDs.

7ad387d

Add custom migration test

75b5794

Enable the custom PM migration test

4263d1f

Add a test for batch custom migration application

d5218a6

Add custom migration tests for projection repository

4fcf821

Merge branch 'master' of https://github.com/SpineEventEngine/core-java …

06274d3

…into fix-migrations

Bump versions

effbfaf

Fix formatting

896d157

Bring back a test proto comment

6c130f0

dmitrykuzmin self-assigned this Sep 2, 2020

dmitrykuzmin requested review from armiol and yuri-sergiichuk September 2, 2020 21:57

yuri-sergiichuk suggested changes Sep 3, 2020

View reviewed changes

dmitrykuzmin and others added 10 commits September 3, 2020 13:33

Reorder fields of Migration.Operation and mark them as @LazyInit

a1fb917

Improve doc of server/src/test/java/io/spine/server/projection/given/…

bb24048

…SetTestProjectionName.java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

Improve doc of server/src/test/java/io/spine/server/procman/given/rep…

e9c6cd4

…o/SetTestProcessName.java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

Merge remote-tracking branch 'origin/fix-migrations' into fix-migrations

adcb6ba

Make custom migration tests more robust

9e9c6a4

Reword a test constant

7f54a15

Expand Migration doc

f93e2a5

Merge branch 'master' of https://github.com/SpineEventEngine/core-java …

e3a7c76

…into fix-migrations

Use Truth8 instead of Truth for optionals

9789df6

Bump version

47baf5c

dmitrykuzmin requested a review from yuri-sergiichuk September 3, 2020 12:31

yuri-sergiichuk suggested changes Sep 3, 2020

View reviewed changes

dmitrykuzmin and others added 8 commits September 3, 2020 18:40

Flip .equals()

6692fc9

Reword Migration doc

9b2e61f

Remove a redundant see section

bd252e2

Use the expected state in column tests

9c2667a

Use the expected entity states for batch migration tests

453e45f

Improve doc of server/src/main/java/io/spine/server/entity/Migration.…

7cc8d0d

…java Co-authored-by: Yuri Sergiichuk <23313616+yuri-sergiichuk@users.noreply.github.com>

Merge remote-tracking branch 'origin/fix-migrations' into fix-migrations

2b1c011

# Conflicts: # server/src/main/java/io/spine/server/entity/Migration.java

Use static imports for Truth8.assertThat(...)

1168c6a

dmitrykuzmin requested a review from yuri-sergiichuk September 3, 2020 17:09

yuri-sergiichuk reviewed Sep 3, 2020

View reviewed changes

yuri-sergiichuk approved these changes Sep 3, 2020

View reviewed changes

Make entity state comparison in migration tests clearer

2c9e533

dmitrykuzmin merged commit 9d1f4cc into master Sep 4, 2020

dmitrykuzmin deleted the fix-migrations branch September 4, 2020 13:24

dmitrykuzmin mentioned this pull request Sep 8, 2020

Release 1.6.0 #1302

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix how the `Migration`s with custom state update are applied #1298

Fix how the `Migration`s with custom state update are applied #1298

dmitrykuzmin commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading

dmitrykuzmin commented Sep 2, 2020

yuri-sergiichuk left a comment

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

dmitrykuzmin commented Sep 3, 2020

yuri-sergiichuk left a comment

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

dmitrykuzmin commented Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk Sep 3, 2020

yuri-sergiichuk left a comment

		assertThat(entityWithColumns.state()
		.getIdString()).isEqualTo(id.toString());

		@@ -738,7 +815,8 @@ void deleteEntityViaMigration() {

		Optional<TestProcessManager> found = repository().find(id);
		assertThat(found.isPresent()).isTrue();

		@@ -238,6 +239,7 @@ private void warnOnNoSystemEventsPosted() {
		private boolean physicallyRemoveRecord;

Fix how the Migrations with custom state update are applied #1298

Fix how the Migrations with custom state update are applied #1298

Conversation

dmitrykuzmin commented Sep 2, 2020 • edited Loading

codecov bot commented Sep 2, 2020 • edited Loading

Codecov Report

dmitrykuzmin commented Sep 2, 2020

yuri-sergiichuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrykuzmin commented Sep 3, 2020

yuri-sergiichuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrykuzmin commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuri-sergiichuk left a comment

Choose a reason for hiding this comment

Fix how the `Migration`s with custom state update are applied #1298

Fix how the `Migration`s with custom state update are applied #1298

dmitrykuzmin commented Sep 2, 2020 •

edited

Loading

codecov bot commented Sep 2, 2020 •

edited

Loading