[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 |
| 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 - |
| Comment by auto [ 08/Feb/11 ] |
|
Author: {u'login': u'rgnitz', u'name': u'Ryan', u'email': u'rgnitz@gmail.com'}Message: initial load - |
| 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 |
| 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(); }
|
| Comment by auto [ 07/Feb/11 ] |
|
Author: {u'login': u'agirbal', u'name': u'agirbal', u'email': u'antoine@10gen.com'}Message: |
| 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. Eventually it would be to get closer to standard db drivers:
|
| 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 - |
| 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: 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? |