Interceptors api #771

rsinukov · 2017-03-18T23:23:11Z

Hi.
There is a proof of concept for interceptors API and sample LoggingInterceptor to see how it works.
Main idea of this PR is to create Interceptor.Chain in every PreparedOperation#executeAsBlocking() method and put in chain RealInterceptor, which will do real job. So, all code in RealInterceptor#intercept() is copypaste of past executeAsBlocking() code.

rsinukov · 2017-03-18T23:33:10Z

closes #542 and part of #541

codecov-io · 2017-03-18T23:33:55Z

Codecov Report

Merging #771 into master will increase coverage by 0.14%.
The diff coverage is 95.09%.

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   96.85%   96.99%   +0.14%     
==========================================
  Files          87       89       +2     
  Lines        2636     2727      +91     
  Branches      296      302       +6     
==========================================
+ Hits         2553     2645      +92     
  Misses         50       50              
+ Partials       33       32       -1

Impacted Files	Coverage Δ
.../com/pushtorefresh/storio/sqlite/StorIOSQLite.java	`100% <ø> (ø)`	⬆️
...ntentresolver/operations/internal/RxJavaUtils.java	`100% <ø> (ø)`	⬆️
...com/pushtorefresh/storio/sqlite/queries/Query.java	`100% <ø> (ø)`	⬆️
...tentresolver/operations/delete/PreparedDelete.java	`100% <ø> (ø)`	⬆️
...storio/sqlite/operations/internal/RxJavaUtils.java	`100% <ø> (ø)`	⬆️
.../pushtorefresh/storio/sqlite/queries/RawQuery.java	`100% <ø> (ø)`	⬆️
...io/contentresolver/operations/put/PreparedPut.java	`100% <ø> (ø)`	⬆️
...torio/sqlite/operations/delete/PreparedDelete.java	`100% <100%> (ø)`	⬆️
...om/pushtorefresh/storio/sqlite/impl/ChainImpl.java	`100% <100%> (ø)`
...ions/delete/PreparedDeleteCollectionOfObjects.java	`97.72% <100%> (+0.05%)`	⬆️
... and 28 more

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 13a5386...26b4dfe. Read the comment docs.

nikitin-da · 2017-03-20T06:59:31Z

And closes #79 =)

nikitin-da

Awesome work man! 👍 Example of usage looks great!

nikitin-da · 2017-03-20T07:01:24Z

settings.gradle

@@ -1,4 +1,4 @@
-include ':storio-common'
+include ':storio-common', ':logginginterceptor'


Maybe move to the new line corresponding to other modules declaration?

nikitin-da · 2017-03-20T07:06:01Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/LoggingInterceptor.java

+    @NonNull
+    private final Logger logger;
+
+    public LoggingInterceptor(@NonNull Logger logger) {


I suppose make it private and create via static method. To make it easer to support compatibility in the future)

nikitin-da · 2017-03-20T07:08:50Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+        interceptors.addAll(registeredInterceptors);
+        interceptors.add(realInterceptor);
+
+        return new ChainImpl(interceptors, 0);


Maybe wrap it with unmodifiable container?

nikitin-da · 2017-03-20T07:09:20Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+    @NonNull
+    private final List<Interceptor> interceptors;
+
+    @NonNull


excess annotation)

nikitin-da · 2017-03-20T07:10:40Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+    @Nullable // can be null on PreparedGetObject
+    @Override
+    public <Result> Result proceed(@NonNull PreparedOperation<Result> operation) {
+        if (index >= interceptors.size()) throw new AssertionError();


IllegalStateException with some message?

nikitin-da · 2017-03-20T07:14:33Z

...o-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/operations/delete/PreparedDelete.java

+    }
+
+    @NonNull
+    protected abstract Interceptor getRealInterceptor();


Maybe getRealCallInterceptor or something like this?

nikitin-da · 2017-03-20T07:23:08Z

storio-sqlite/src/test/java/com/pushtorefresh/storio/sqlite/integration/InterceptorTest.java

+    }
+
+    private void checkInterceptorsCalls() {
+        assertThat(callCount.get()).isEqualTo(2);


Add inOrder verification?

Strange, in my machine tests fail when I mock Interceptor with some internal Mockito error. Can you check it on your machine pls?

Sorry, I should update robolectric to support mockito2 (robolectric/robolectric#2677). I will done it soon.

nikitin-da · 2017-03-20T07:25:21Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/StorIOSQLite.java

@@ -177,6 +178,9 @@
    @NonNull
    public abstract LowLevel lowLevel();

+    @NonNull


It seems we will need javadoc for it and for interceptor and chain too=)

geralt-encore

Good stuff!

geralt-encore · 2017-03-22T21:27:22Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+    @NonNull
+    private final int index;
+
+    private int calls;


Can be changed to just a boolean flag since according to the code we don't really care about actual number of calls.

Actually we check that it wasn't called more than once

+1 for boolean)
And should it be thread safe?

geralt-encore · 2017-03-22T21:31:51Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+        }
+
+        // Call the nextChain nextInterceptor in the chain.
+        final ChainImpl nextChain = new ChainImpl(interceptors, index + 1);


What about passing an Iterator so we can get rid of the index?

geralt-encore · 2017-03-22T21:38:44Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/impl/ChainImpl.java

+        final Result result = nextInterceptor.intercept(operation, nextChain);
+
+        // Confirm that the nextChain nextInterceptor made its required call to chain.proceed().
+        if (index + 1 < interceptors.size() && nextChain.calls != 1) {


Won't the same checks be performed inside proceed of nextChain?

geralt-encore · 2017-03-22T21:43:54Z

...e/src/main/java/com/pushtorefresh/storio/sqlite/operations/delete/PreparedDeleteByQuery.java

+    @NonNull
+    @Override
+    protected Interceptor getRealInterceptor() {
+        return new RealInterceptor();


Can be just an anonymous class (and below)

I think that standalone class is more readable. @nikitin-da what do you think?

A agree that variant with standalone class will be more readable.
Also if we will decided some days to make it public, there will be less things to change=)

geralt-encore · 2017-03-22T21:53:04Z

storio-common/src/main/java/com/pushtorefresh/storio/operations/PreparedOperation.java

@@ -62,4 +62,7 @@
    @NonNull
    @CheckResult
    Single<Result> asRxSingle();
+
+    @NonNull
+    Object getData();


What about providing some common interface or wrapper to avoid passing plane object?

With interface it would be impossible to use third side classes.
We can add generic param to all prepared operation, if you think that it worth doing.

I think we need to try generics in prepared operations, Object seems too weird…

Generics impossible for PreparedGetOperation, cause it can have either Query or RawQuery. We can create common interface for them, but what a point? Anyway, it will not add any details, cause user will see common generic parameter inside Interceptor which is same as seeing Object.

Ok, then you deal with all complaints from users :D

artem-zinnatullin

JESOS, this is huge, but looks good! Minor comments

artem-zinnatullin · 2017-03-26T20:33:18Z

storio-common/src/main/java/com/pushtorefresh/storio/operations/PreparedOperation.java

@@ -62,4 +62,7 @@
    @NonNull
    @CheckResult
    Single<Result> asRxSingle();
+
+    @NonNull
+    Object getData();


I think we need to try generics in prepared operations, Object seems too weird…

artem-zinnatullin · 2017-03-26T20:34:58Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/Interceptor.java

+    <Result> Result intercept(@NonNull PreparedOperation<Result> operation, @NonNull Chain chain);
+
+    /**
+     * Encapsulates logic of getting from one interceptor to the next one.


of proceeding from one interceptor to another

artem-zinnatullin · 2017-03-26T20:35:34Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/LoggingInterceptor.java

+    @Nullable
+    @Override
+    public <Result> Result intercept(@NonNull PreparedOperation<Result> operation, @NonNull Chain chain) {
+        final long startMillis = System.currentTimeMillis();


SystemClock.elapsedRealtime()

artem-zinnatullin · 2017-03-26T20:35:39Z

storio-sqlite/src/main/java/com/pushtorefresh/storio/sqlite/LoggingInterceptor.java

+
+        final Result result = chain.proceed(operation);
+
+        final long endMillis = System.currentTimeMillis();


SystemClock.elapsedRealtime()

rsinukov · 2017-04-09T22:08:02Z

@artem-zinnatullin we with @nikitin-da decided to add generic param to PreparedOperation

artem-zinnatullin · 2017-04-10T08:59:34Z

...lite/src/test/java/com/pushtorefresh/storio/sqlite/operations/get/PreparedGetCursorTest.java

@@ -28,6 +29,28 @@
 public class PreparedGetCursorTest {

    @Test
+    public void shouldThrowInNoQueryOrRawQueryIsSet() {


artem-zinnatullin · 2017-04-17T00:15:08Z

storio-common/src/main/java/com/pushtorefresh/storio/operations/PreparedOperation.java

@@ -62,4 +62,7 @@
    @NonNull
    @CheckResult
    Single<Result> asRxSingle();
+
+    @NonNull
+    Data getData();


So basically this does not help interceptors to be type-safe but gives a way to get data without need in casting PreparedOperation to some particular implementation.

I guess we'll have to ship it as 2.x then since it's addition to an interface which is a breaking change (when I was giving greenlight to that I was thinking just about , Data> generic which is not breaking but forgot that it'll add a method)

…inserts and updates

nikitin-da requested changes Mar 20, 2017

View reviewed changes

geralt-encore reviewed Mar 22, 2017

View reviewed changes

artem-zinnatullin requested changes Mar 26, 2017

View reviewed changes

artem-zinnatullin approved these changes Apr 3, 2017

View reviewed changes

rsinukov added 7 commits April 10, 2017 02:43

Interceptors API (proof of concept)

2f6360a

Tests for interceptors

0ca2047

Review fixes

a45fcba

Increase test coverage

9a6edd0

Add javadocs

ea46c4f

More review fixes

7e87d2c

Add generic param for PreparedOperation data

83e4701

artem-zinnatullin requested changes Apr 10, 2017

View reviewed changes

Rebase problems fixed

19a45a5

rsinukov force-pushed the interceptors branch from 055a92c to 19a45a5 Compare April 10, 2017 11:54

artem-zinnatullin approved these changes Apr 17, 2017

View reviewed changes

Tests added

6b686e5

rsinukov force-pushed the interceptors branch from 200fd46 to 6b686e5 Compare April 17, 2017 16:28

artem-zinnatullin added this to the v2.0.0 milestone May 14, 2017

artem-zinnatullin added the enhancement || feature label May 14, 2017

Increase coverage: add tests on empty collection put and put without …

0956a08

…inserts and updates

nikitin-da force-pushed the interceptors branch from 1df6e44 to 0956a08 Compare June 12, 2017 21:46

nikitin-da approved these changes Jun 12, 2017

View reviewed changes

nikitin-da added 2 commits June 13, 2017 01:03

Merge branch 'master' into interceptors

fa3f058

Merge branch 'master' into interceptors

26b4dfe

artem-zinnatullin approved these changes Jul 6, 2017

View reviewed changes

artem-zinnatullin merged commit 9907616 into pushtorefresh:master Jul 6, 2017

nikitin-da mentioned this pull request Jul 12, 2017

Fix case with different classes having fields with the same names #803

Merged

artem-zinnatullin mentioned this pull request Jul 27, 2017

Interceptors API #542

Closed

		@@ -1,4 +1,4 @@
		include ':storio-common'
		include ':storio-common', ':logginginterceptor'


		final Result result = chain.proceed(operation);

		final long endMillis = System.currentTimeMillis();

Interceptors api #771

Interceptors api #771

Conversation

rsinukov commented Mar 18, 2017

rsinukov commented Mar 18, 2017

codecov-io commented Mar 18, 2017 • edited Loading

Codecov Report

nikitin-da commented Mar 20, 2017

nikitin-da 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geralt-encore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsinukov Mar 26, 2017 • edited Loading

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

Choose a reason for hiding this comment

rsinukov Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-zinnatullin 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

rsinukov commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Mar 18, 2017 •

edited

Loading

rsinukov Mar 26, 2017 •

edited

Loading

rsinukov Mar 29, 2017 •

edited

Loading