Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
ValeriusGC opened this issue Dec 7, 2017 · 5 comments
Closed
Assignees
Milestone

Comments

@ValeriusGC
Copy link
Contributor

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
Copy link
Collaborator

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?

@nikitin-da nikitin-da self-assigned this Dec 7, 2017
@nikitin-da nikitin-da added this to the v3.0.0 milestone Dec 7, 2017
@ValeriusGC
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Member

artem-zinnatullin commented Dec 7, 2017 via email

@nikitin-da
Copy link
Collaborator

Closed with #864

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants