pushtorefresh / storio

Reactive API for SQLiteDatabase and ContentResolver.
Apache License 2.0
2.54k stars 182 forks source link

Why there is collision between @Nullable annotation and real explanation? #863

Closed ValeriusGC closed 6 years ago

ValeriusGC commented 6 years ago

Hello. Inspection warns me about possible NPE on insertedId() in the chain of

        return storIo
                .put()
                .object(table)
                .prepare()
                .executeAsBlocking()
                .insertedId(); // Here is possible NPE

When i follow to the source i see @Nullable annotation and @return non-null results of Put Operation. explanation. So, how one can interpret it?

    /**
     * Executes Put Operation immediately in current thread.
     * <p>
     * Notice: This is blocking I/O operation that should not be executed on the Main Thread,
     * it can cause ANR (Activity Not Responding dialog), block the UI and drop animations frames.
     * So please, call this method on some background thread. See {@link WorkerThread}.
     *
     * @return non-null results of Put Operation.
     */
    @WorkerThread
    @Nullable
    public final Result executeAsBlocking() {
        return buildChain(storIOSQLite.interceptors(), getRealCallInterceptor())
                .proceed(this);
    }
nikitin-da commented 6 years ago

Hi, @ValeriusGC The reason why it was changed to nullable is that user can add some custom interceptor that may return null as a result So there is no guarantee that result is notNull

Of course in common case (for all operations except get().object()) result should not be null. So I guess we can add runtime nullability check into executeAsBlocking

@geralt-encore what do you think?

ValeriusGC commented 6 years ago

Just a minute. According above chain semantic it is allowed to process result of previous operation executeAsBlocking() in the next step, doesn't it? So there is the source of potential NPE when null-result is allowed here. IMHO it is some kind of trap to place @Nullable in the the probable continuation of the call chain. Maybe not allowing null would be the better strategy...

nikitin-da commented 6 years ago

Actually we can't prohibit nulls at all because we use the same interceptor interface for all operations. And one of them can return null

artem-zinnatullin commented 6 years ago

We can, we can remove nullability annotation from interceptor to leave it undefined

On Thu, Dec 7, 2017 at 1:07 AM Dmitrii Nikitin notifications@github.com wrote:

Actually we can't prohibit nulls at all because we use the same interceptor interface for all operations https://github.com/pushtorefresh/storio/blob/d968a579c0094179dc26747c2ea01a08d9a36286/storio-sqlite/src/main/java/com/pushtorefresh/storio3/sqlite/impl/DefaultStorIOSQLite.java#L267. And one of them can return null

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/pushtorefresh/storio/issues/863#issuecomment-349906186, or mute the thread https://github.com/notifications/unsubscribe-auth/AA7B3Hk_mQhU8BrRGB_90AQKjqL6qtPJks5s96q4gaJpZM4Q4-1m .

nikitin-da commented 6 years ago

Closed with #864