powered by Jive Software

NPE in o.x.p.JID


#1

In

public JID(String node, String domain, String resource, boolean skipStringprep)

the docs say that IllegalArgumentException is thrown if the JID is not valid.

Nonetheless, four lines after that

throw new NullPointerException(“Domain cannot be null”);


#2

Individual parts of a JID can be null, except for the domain-part, which always has to be a non-null value. This is why a null-check is done on domain, before any of the validation takes place. If the provided argument are invalid (e.g: do not pass stringprep), IllegalArgumentExceptions are thrown. I’ve updated the javadoc to make mention of the possible NullPointer.


#3

Yep, but what would be wrong about throwing an IllegalArgumentException as well? In the end, it is an illegal argument and that way I could simply catch one Exception instead of catching a superclass or having several catch blocks.


#4

My biggest concern is that it’d break backwards compatibility. Although I do not dislike your idea of throwing the same exception for both types of problems (I agree with you that the current implementation is a bit messy), I don’t want to risk breaking other projects over a relatively minor, mostly cosmetic change.


#5

Hi Guus,

I understand that concern, but still think that Tinder should handle it (more) correctly. I doubt that there really is code that handles the NPE and IAE differently, but rather think it either catches both as RunTimeException or Exception, ignores both or handles both equally. But I may be wrong, of course. From the API perspective, I would argue that this is a bug fix given the Javadocs (as released).

BTW, I even think you could add “throws IllegalArgumentException” to the method, even if it does not really change requirements for the caller (IMHO).

With regard to cosmetic, consider me as a new tinder user. I do want to check whether the constructor accepted the parts for a jid, so I catch IAE and suddenly NPEs fly around. I think adding NPE to the docs makes it uglier…

Thanks for considering :slight_smile:


#6

Over the past few days, I’ve given this some thought. I didn’t change my mind though. Each of the thrown exceptions has a very specific cause and, arguably, require a different way of handling:

A thrown IllegalArgumentException is thrown if the provided arguments contain illegal characters or are otherwise incompatible with the XMPP specs. Typically, a user has entered incorrect data. These exceptions indicate a problem that should eventually be reported back to the end-user that provided the input data (probably using an XMPP error of type modify).

A thrown NullPointerException indicates a bug: somewhere, code allows a JID to be constructed without a domain. These kind of problems are typically not triggered by end users, but by the code processing or parsing data. These exceptions indicate a problem that should be reported back to the maintainers of the software and/or environment (note that end-users should be warned too, if appropiate. I’d expect errors with internal-server-error conditions here).


#7

Hey Guus,

thanks for giving it a chance :slight_smile: Maybe it is a matter of choice in the end, so I like a definite answer and hope that you have come to conclusions that help in other areas of the library as well.

Ciao,

– andi5


#8

We will keep the code as-is. If you have suggestion on how to make it clearer for a developer through javadoc or other modifications (you’re working from that perspective which makes you an expert ), let me know.