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

Invalid @NonNullApi and null return #871

Closed
knalli opened this issue Dec 14, 2018 · 3 comments
Closed

Invalid @NonNullApi and null return #871

knalli opened this issue Dec 14, 2018 · 3 comments

Comments

@knalli
Copy link
Contributor

knalli commented Dec 14, 2018

Hi,

I have noticed an invalid nullable situation within spring-rabbit's API.

At least the method org.springframework.amqp.rabbit.core.RabbitAdmin#getQueueProperties is nullable (I think it was nullable alway as I'm using this for explicit queue management on startup for years) as the code clearly shows (in the local exception handling) as well as the documentation clearly states

Returns 3 properties {@link #QUEUE_NAME}, {@link #QUEUE_MESSAGE_COUNT},
{@link #QUEUE_CONSUMER_COUNT}, or null if the queue doesn't exist.

However, the package contains a @NonNullApi also. And as I had understand this correctly, either this one is not correct, or there is a @Nullable missing, or the code should not return null.

The issue got into my attention as IntelliJ hints about a redundant null check. Fortunately, I have checked this.

@artembilan
Copy link
Member

This is bug. The method and its contract in the AmqpAdmin has to be marked with the @Nullable.

Feel free to provide a fix on the matter!
Also we won't mind if you review all other AmqpAdmin methods for similar flaw.

Thanks!

knalli added a commit to knalli/spring-amqp that referenced this issue Dec 14, 2018
@knalli
Copy link
Contributor Author

knalli commented Dec 14, 2018

Sure. PR is there. I'm hoping this was right for this project and I had not missing some (non technical).

btw: I have noticed the same issue with org.springframework.amqp.rabbit.core.ChannelCallback#doInRabbit. It is defined not to return null, but this is done a lot.

@artembilan
Copy link
Member

That one has to be fixed as well. There is no obligation from end-user to return something specific from that lambda implementation.
More over I found usage like this:

execute(channel -> {
	channel.queueDeclarePassive(Address.AMQ_RABBITMQ_REPLY_TO);
	return null;
});

So, this one has to be @Nullable as well. See OperationsCallback with similar signature.

Thanks

knalli added a commit to knalli/spring-amqp that referenced this issue Dec 14, 2018
knalli added a commit to knalli/spring-amqp that referenced this issue Dec 14, 2018
artembilan pushed a commit that referenced this issue Dec 14, 2018
Resolves #871

* Add missing `@Nullable`

Relates #871

* Fixup order of imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants