[JAVA-1896] NOP used where cast expected in Document.get(String,Class) Created: 18/Jul/15 Updated: 01/Apr/16 Resolved: 26/Aug/15 |
|
| Status: | Closed |
| Project: | Java Driver |
| Component/s: | BSON |
| Affects Version/s: | 3.0.0 |
| Fix Version/s: | 3.1.0 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Michael Brown | Assignee: | Jeffrey Yemin |
| Resolution: | Done | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Description |
|
public <T> T get(final Object key, final Class<T> clazz) This implementation does a 'fake' cast of (T) which discards the 'clazz' parameter altogether. value = (T) someObject is a NOP in Java, and that is why somebody has put a warnings suppression on this method. It's an easy fix, and I am happy to do it myself, but I don't know your project bylaws etc on user contribution and review processes. Please advise. |
| Comments |
| Comment by Jeffrey Yemin [ 07/Oct/15 ] | ||||||||||||||||||||
|
Released in 3.1.0 | ||||||||||||||||||||
| Comment by Githook User [ 26/Aug/15 ] | ||||||||||||||||||||
|
Author: {u'username': u'jyemin', u'name': u'Jeff Yemin', u'email': u'jeff.yemin@10gen.com'}Message: Now properly throwing a ClassCastException from org.bson.Document.get(java.lang.Object, java.lang.Class<T>), as documented. | ||||||||||||||||||||
| Comment by Ross Lawley [ 21/Jul/15 ] | ||||||||||||||||||||
|
Thanks for following up supersoftcafe - I see the issue now. I've reopened and will fix the code to explicitly throw a ClassCastException if the value class is not assignable from clazz. That should fix the issue and still allow the API to be flexible enough for users needs. | ||||||||||||||||||||
| Comment by Michael Brown [ 20/Jul/15 ] | ||||||||||||||||||||
|
I think that a stack trace that states that a class cast exception occurs where no class cast is visible in the code is less than clear, and I have seen in teams in previous employment the man hours wasted when the head scratching starts. However, on that point alone I'd leave it and move on. The second sticking point is that the compiler doesn't always insert a class cast, so the JavaDoc noted ClassCastException may never occur, and this is more worrying. Take please the following contrived example (substitute Integer for any other supported class):
Which produces this byte code:
No casts in there, and we now have the potential of a Map of Integer polluted with other types. Eventually there might be a cast exception, depending on what the caller is doing, but it'll be so far away from the cause as to be a pain to debug. I believe that the greatest clarity is achieved by keeping cause+effect closely tied, in other words, let the exception occur at the earliest opportunity. However, if you really want this behaviour, then I ask that you at least change the JavaDoc, because the method doesn't throw a ClassCastException. | ||||||||||||||||||||
| Comment by Ross Lawley [ 20/Jul/15 ] | ||||||||||||||||||||
|
HI supersoftcafe thanks for the ticket, You are correct, the clazz parameter is only there to provide the type to explicitly return, the method is unchecked because it can cause a runtime error when trying to cast to a type the value is not. The method is a helper function to help users interact with Documents. The Document class is an implementation of Map<String, Object> and the type of any given value will be seen as Object. It is not a typesafe implementation and we cast to expected type, which can produce runtime errors. The native map interface only mandates that we have to implement a get method of the following type: Object get(String key). As returning Object isn't terribly helpful, we added a number of other helper methods eg: getLong(key), getBoolean(key) which do a cast on the underlying result of a get. This is fine for all value types that map to Bson. However, Document can take any value, for instance I might have a specific codec to handle the serialization and deserialization of my User class. In situations like this the get(key, clazz<T>) method is helpful as it returns the value as the type T and not Object but it does require you pass in Class<T> even though its only used to get the type. eg:
It's a convenience method so users don't have to do the cast themselves - similar to getLong(key) and friends. We could move the unchecked code by calling clazz.cast but it has the same behaviour, with the downside of moving the stacktrace in the case of exceptions to the cast method, rather than explicitly being on the get method. For these reasons I'm going to close this ticket as "Works As Designed". If I've missed something please follow up and I'll review! |