Lifecycle flags for entities #372

alexander-yevsyukov · 2017-03-06T22:21:23Z

This PR:

Removes markArchived() and markDeleted() methods in repositories and storages.
Renames Visibility proto type into LifecycleFlags.
Introduces EntityWithLifecycle interface.
Introduces StorageWithLifecycleFlags interface.
Renames VisibilityField into LifecycleFlagField.

It's already available on the level of the package.

Also removed `markArchived()` and `markDeleted()` methods (which duplicate `writeVisibility()` methods).

…on-entity-level

codecov · 2017-03-06T22:24:17Z

Codecov Report

Merging #372 into master will increase coverage by 0.02%.
The diff coverage is 94.49%.

@@             Coverage Diff              @@
##             master     #372      +/-   ##
============================================
+ Coverage     93.18%   93.21%   +0.02%     
+ Complexity     2327     2314      -13     
============================================
  Files           232      231       -1     
  Lines          7615     7589      -26     
  Branches        559      561       +2     
============================================
- Hits           7096     7074      -22     
  Misses          390      390              
+ Partials        129      125       -4

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d283d4...24ce525. Read the comment docs.

alexander-yevsyukov · 2017-03-06T23:50:11Z

@armiol, PTAL.

armiol

@alexander-yevsyukov please see my comments.
Let's discuss the VisibleEntity#visibilityChanged().

armiol · 2017-03-07T10:55:04Z

server/src/main/java/org/spine3/server/entity/VisibleEntity.java


 /**
- * Routines common for all mismatches.
+ * An entity which has visibility.


Can we reference some definition of visibility? Or something that would allow readers to catch up on the subject.

armiol · 2017-03-07T10:55:35Z

server/src/main/java/org/spine3/server/storage/RecordStorage.java

+                                               .build();
+            write(id, updated);
+        } else {
+            // The AggregateStateId is a special case, which is not handled by the Identifier class.


Identifiers?

The code that creates the string form of an ID is actually in the Identifier.Type enum.

The AggregateStateId being a plain Java class (not one of primitive wrappers that we support, and not a Message) doesn't fall into the case in Identifier.toString(), which causes the error.

I didn't want to extend the Identifier for this particular case, hence the comment.

armiol · 2017-03-07T10:57:04Z

server/src/main/java/org/spine3/server/entity/VisibleEntity.java

+    /**
+     * Verifies whether visibility of the entity changed since its initialization.
+     */
+    boolean visibilityChanged();


That's an odd interface method. Seems more like a utility method of an abstract class-level.

armiol · 2017-03-07T10:57:46Z

server/src/main/java/org/spine3/server/entity/VisibleEntity.java

-        checkArgument(!expected.equals(actual), ERR_CANNOT_BE_EQUAL);
-    }
+    /**
+     * Verifies whether visibility of the entity changed since its initialization.


Should probably be Tells whether ..., since if fact there is verification done per se, i.e. no exceptions thrown.

armiol · 2017-03-07T10:58:57Z

server/src/main/java/org/spine3/server/storage/StorageWithVisibility.java

+     *
+     * <p>This method returns {@code Optional.absent()} if none of the
+     * flags of visibility flags were set before. This means that
+     * the entity is visible to the regular queries.


While it is perfectly clear to me, the regular queries may cause some questions. What are they? Are there irregular queries? Do the irregular queries act differently in the past? Etc.

I'll remove the last sentence. It repeats what is already said in Visibility.

…on-entity-level

alexander-yevsyukov · 2017-03-07T18:17:01Z

@armiol, PTAL.

…on-entity-level

alexander-yevsyukov · 2017-03-07T20:05:46Z

@armiol, PTAL. Specific repository views will be in an separate PR.

Tested `Preconditions` for null tolerance

The converter's output is only `null` if the input is `null`. Since the passed entity isn't null, the check is redundant.

armiol

@alexander-yevsyukov LGTM except for a few minor comments and one logical duplicate, that seems worth discussing. Please see my comments.

armiol · 2017-03-08T10:21:24Z

server/src/main/java/org/spine3/server/storage/StorageWithLifecycleFlags.java


 /**
- * Routines common for all mismatches.
+ * A storage that allows to update lifecycle flags of entities.


Let's reference the LifecycleFlags via {@link...} here as well.

armiol · 2017-03-08T10:21:58Z

server/src/main/java/org/spine3/server/storage/memory/InMemoryStandStorage.java

+        final Map<AggregateStateId, EntityRecord> resultMap = Maps.filterKeys(
+                allRecords,
+                new Predicate<AggregateStateId>() {
+                    @Override
            public boolean apply(@Nullable AggregateStateId stateId) {


Looks unformatted. Maybe, just a GitHub bug.

armiol · 2017-03-08T10:24:39Z

server/src/main/proto/spine/server/entity/entity.proto

-//
-message Visibility {
+// The lifecycle flags of an entity.
+message LifecycleFlags {


It's a pity we have to duplicate LifecycleFlags proto message and LifecycleFlagField class structure.

Can we avoid it somehow? ATM, we'll have to append the same changes to these two classes if any new flags are introduced.

It would require having a general mechanism of transforming a message into a set of storage fields. And detecting simple cases like just boolean flags like this. Kinda Hibernate :) I'd say, it should be a separate PR anyway.

alexander-yevsyukov · 2017-03-08T12:26:58Z

@armiol, PTAL.

Get rid of `Tuple`

armiol · 2017-03-09T15:16:28Z

@alexander-yevsyukov as discussed vocally, the storage mechanism will be updated and refactored.
So LGTM for now.

alexander-yevsyukov added 6 commits March 6, 2017 21:12

Remove markXxx() methods from AggregateRepository

8026035

Remove duplicating @SPI annotations

0f832e6

It's already available on the level of the package.

Introduce VisibleEntity and StorageWithVisibility

6448aea

Also removed `markArchived()` and `markDeleted()` methods (which duplicate `writeVisibility()` methods).

Improve code layout

a5954e6

Add Javadoc and improve code layout

458b3e4

Merge remote-tracking branch 'remotes/origin/master' into visibility-…

f1f4b26

…on-entity-level

alexander-yevsyukov self-assigned this Mar 6, 2017

alexander-yevsyukov added 6 commits March 7, 2017 00:34

Test Visibility factory methods in Tests

09e2e35

Remove unused code in TenantRecords

96492d6

Remove Mismatches utility class

6304b7b

Test visibility of null records

b63888d

Test loading visibility

408509a

Test reading with field mask

de36ebf

alexander-yevsyukov requested a review from armiol March 6, 2017 23:49

Fix missing dot.

d6b2c7d

armiol requested changes Mar 7, 2017

View reviewed changes

alexander-yevsyukov added 3 commits March 7, 2017 20:04

Merge remote-tracking branch 'remotes/origin/master' into visibility-…

2f59011

…on-entity-level

Rename the visibility mod. check method

021ff76

Fix Javadoc

dd4c5e9

alexander-yevsyukov added 3 commits March 7, 2017 21:08

Rename Visibility to LifecycleFlags

b2e5cf0

Rename VisibilityField to LifecycleFlagField

2035a05

Fix code layout

991fda6

alexander-yevsyukov changed the title ~~Visibility on entity level~~ Lifecycle flags for entities Mar 7, 2017

alexander-yevsyukov added 3 commits March 7, 2017 21:16

Merge remote-tracking branch 'remotes/origin/master' into visibility-…

38b9bc1

…on-entity-level

Move predicates under EntityWithLifecycle

e35a976

Simplify flag predicates dependency

31b74d5

alexander-yevsyukov added 6 commits March 8, 2017 04:28

Simplify int and long equality checking

18cbcee

Tested `Preconditions` for null tolerance

Pack entity ID into EntityRecord

48c56e0

Make DefaultEntityFactory package-access instead of public

719c810

Get rid of Tuple class in entity storage conversion

2c55ec2

Remove done TODO

23ee857

Advance Spine verison to 0.8.19-SNAPSHOT

385e9a8

alexander-yevsyukov mentioned this pull request Mar 8, 2017

Get rid of Tuple #376

Merged

alexander-yevsyukov added 2 commits March 8, 2017 06:52

Test equality of FieldEnricher

ff4f139

Remove redundant check for null.

c955d00

The converter's output is only `null` if the input is `null`. Since the passed entity isn't null, the check is redundant.

armiol requested changes Mar 8, 2017

View reviewed changes

alexander-yevsyukov added 2 commits March 8, 2017 14:17

Update Javadoc with link

daa46e5

Improve code layout

e305e9a

Merge pull request #376 from SpineEventEngine/use-entity-id-from-record

24ce525

Get rid of `Tuple`

armiol approved these changes Mar 9, 2017

View reviewed changes

alexander-yevsyukov merged commit 4d3b1fe into master Mar 9, 2017

alexander-yevsyukov deleted the visibility-on-entity-level branch March 9, 2017 15:19

Lifecycle flags for entities #372

Lifecycle flags for entities #372

Uh oh!

Conversation

alexander-yevsyukov commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alexander-yevsyukov commented Mar 6, 2017

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-yevsyukov commented Mar 7, 2017

Uh oh!

alexander-yevsyukov commented Mar 7, 2017

Uh oh!

armiol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-yevsyukov commented Mar 8, 2017

Uh oh!

armiol commented Mar 9, 2017

Uh oh!

Uh oh!

alexander-yevsyukov commented Mar 6, 2017 •

edited

Loading

codecov bot commented Mar 6, 2017 •

edited

Loading