[JAVA-265] Java driver throws a lot of undeclared/non MongoException runtime exceptions Created: 03/Feb/11  Updated: 25/Jul/16  Resolved: 25/Jul/16

Status: Closed
Project: Java Driver
Component/s: API
Affects Version/s: 2.4
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Ryan Nitz Assignee: Unassigned
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified


 Description   

The following runtime exceptions are thrown by the Java driver.

They make handling database failures incredibly difficult.

Essentially, the only thing the Java driver should throw are a) declared exceptions (e.g., IOException) or MongoException (or a subclass of).

Let me know if you are OK with these changes and I'll modify the driver.

RuntimeException
IllegalArgumentException
IllegalStateException
UnsupportedOperationException



 Comments   
Comment by Jeffrey Yemin [ 28/Apr/12 ]

Looking at the current code base, the uses of IllegalArgumentException, IllegalStateException, and UnsupportedOperationException seem for the most part appropriate. There are a quite a few RuntimeException throws that could still be replaced with a more appropriate exception.

Comment by auto [ 08/Feb/11 ]

Author:

{u'login': u'rgnitz', u'name': u'Ryan', u'email': u'rgnitz@gmail.com'}

Message: converted RuntimeException to BSONException - JAVA-265
https://github.com/mongodb/mongo-java-driver/commit/6125995550c6363ba8b63e824f97f227705b749c

Comment by auto [ 08/Feb/11 ]

Author:

{u'login': u'rgnitz', u'name': u'Ryan', u'email': u'rgnitz@gmail.com'}

Message: initial load - JAVA-265
https://github.com/mongodb/mongo-java-driver/commit/5cd12184abb57f585f2592f5f05f02528a91e5f7

Comment by Keith Branton [ 08/Feb/11 ]

> ppl who choose to use Java expect checked exception

Not sure I agree with this statement (only been using java commercially for about 12 years though). There is plenty guidance on where unchecked exceptions are appropriate, and the platform code is full of them. i.e. http://www.javapractices.com/topic/TopicAction.do?Id=129

I think it is completely reasonable for MongoException to be checked. Most java programmers will abstract out the data layer anyway, so the checks only really need to be done in the data layer.

Comment by Antoine Girbal [ 08/Feb/11 ]

Having worked with checked exceptions in all libraries I used for the past 5 years, it feels very unsafe to have unchecked exception, esp for a db driver .
There is no downside to checked exception as long as exceptions are well organized and thrown where it makes sense.
If you use an IDE you actually end up doing much less typing since it can autofill try/catch block or throws.
To make an exception bubble up, just adds "throws" to the signature.
Right now users have no idea what we throw and I've just closed 2 tickets in past day where ppl got exception in places where doc didnt declare it.
We shouldnt rely on perfect doc to make user code safe.
I'm trying to build some larger client app to test out driver and when you have many functions it's tough to put try/catch all around by yourself and you end up with trial and error process.
Client app may run well for a while but then stumbles because of missed catch block.
I guess there has been long lived debate on checked vs unchecked but in java world that has been settled long ago (i.e. ppl who choose to use Java expect checked exception)...

Comment by Eliot Horowitz (Inactive) [ 07/Feb/11 ]

That construct was probably when MongoException was typed, so the try/catch can just go.

Comment by Ryan Nitz [ 07/Feb/11 ]

Are there any other references to this outside of DBCursor.java? I agree that doesn't make sense...

try

{ return _next(); }


catch ( MongoException e )

{ throw new MongoInternalException( "couldn't get next element" , e ); }

Comment by auto [ 07/Feb/11 ]

Author:

{u'login': u'agirbal', u'name': u'agirbal', u'email': u'antoine@10gen.com'}

Message: JAVA-265: Java driver throws a lot of undeclared/non MongoException runtime exceptions
https://github.com/mongodb/mongo-java-driver/commit/76e13f04f1bff54f7fa025a353b72be38bb07819

Comment by Ryan Nitz [ 07/Feb/11 ]

Remove the generic RuntimeException or all runtime exceptions?

The runtime exceptions make perfect sense for people who let their exceptions bubble up to another level. You don't need those blocks unless you want to act upon a particular exception.

If they are catching MongoInternalException specifically it will still catch, unless they are catch MongoException first.

+1 for more error codes

-1 for a checked exceptions - The original driver had them and I was very happy to see them go.

Comment by Antoine Girbal [ 07/Feb/11 ]

We have a bunch of the following around:

try

{ ... }

catch ( MongoException me )

{ throw new MongoInternalException( "can't do getmore" , me ); }

It didnt really make sense before, and now even less since MongoInternalException extends MongoException.
Would love to get rid of these blocks in 2.5.
It can potentially affect some users if they were trying to catch MongoInternalException specifically.
But since we use Exception so loosely, and it was not documented, pretty sure everyone just does "catch(Exception e)".

Eventually it would be to get closer to standard db drivers:

  • use MongoException to represent server or client error, with a specific error code
  • make MongoException checked so that ppl know when things can fail.
  • try to remove RuntimeException as much as possible.
Comment by auto [ 04/Feb/11 ]

Author:

{u'login': u'rgnitz', u'name': u'Ryan', u'email': u'rgnitz@gmail.com'}

Message: changed to extend MongoException - JAVA-265
https://github.com/mongodb/mongo-java-driver/commit/c08f9c4baf063a28917d3f77fc36caa3702ed7ca

Comment by Eliot Horowitz (Inactive) [ 04/Feb/11 ]

making MongoInternalException extend MongoException is fine

Comment by Ryan Nitz [ 04/Feb/11 ]

Ok... some progress

How does everyone feel about changing MongoInternalException to extend MongoException?

MongoInternalException is typically thrown if there is an IOException or if there is another problem communicating with the server.

Comment by Eliot Horowitz (Inactive) [ 04/Feb/11 ]

these:
IllegalArgumentException
IllegalStateException
UnsupportedOperationException

are completely kosher in my mind and should not be changed.

generic RuntimeExceptions can usually be made either one of those or IOException

basically these: git grep RuntimeException src/main/ | grep throw

can submit a patch to change some of those, but that's it.

Comment by Scott Hernandez (Inactive) [ 04/Feb/11 ]

I think you are correct that better javadocs need to be added (for specific exceptions, not generic ones) and a better exceptions hierarchy needs to be maintained.

Comment by Keith Branton [ 03/Feb/11 ]

@Ryan, please disregard my previous comment, which I have deleted now since I notice MongoException is unchecked anyway.

I'm not in favor of this proposal.

In your example you could simply wrap the mongo call in its own try/catch if you want to handle all exceptions from mongo separately from the exceptions in other code. That is what I do - and what I expect most users do.

Guaranteeing that the java driver can't ever throw NullPointerException, ArrayIndexOutOfBoundsException, or any other RuntimeException subclasses that get thrown by platform code would be very difficult.I suppose the contents of every public method in the driver could be wrapped in a try/catch, but that would be ugly and inefficient.

You really can't guarantee, now and forever in the future, that no unchecked exceptions besides MongoException will ever be thrown by the driver - so this change is pointless.

Comment by Ryan Nitz [ 03/Feb/11 ]

Yes... right now, most of these runtime exceptions are not documented so it is difficult to tell when something is going to come through.

It's important for people to be able to easily identify and handle persistence issues based on exception types.

try

{ // Make a call to mongo // Make a call to some other app/service/lib }

catch (MongoException m)

{ // handle a mongo error - persist data to flat file }

catch (MongoInternalException mie)

{ // this should extend MongoException // handle a different mongo error - persist data to flat file }

catch (Throwable t)

{ // You can't handle these runtime errors with WriteResult // handle some other mongo error, is this in my app or another library? I don't know where the error originated from and i have to handle them differently }
Comment by Scott Hernandez (Inactive) [ 03/Feb/11 ]

So you think that a bad/illegal argument should result in a MongoException?

What do you plan to do with those runtime exceptions?

Generated at Thu Feb 08 08:51:52 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.