[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.

JAVA-1896
Branch: master
https://github.com/mongodb/mongo-java-driver/commit/0bfdd813201704008ecae64dde4754a8c75083f8

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):

public Map<String, Integer> getMapOfIntegers() {
    Map<String, Integer> map = new HashMap<>();
    map.put("fred", doc.get("fred", Integer.class));
    // Repeat for each one needed
    return map;
}

Which produces this byte code:

       0: new           #3                  // class java/util/HashMap
       3: dup
       4: invokespecial #4                  // Method java/util/HashMap."<init>":()V
       7: astore_1
       8: aload_1
       9: ldc           #5                  // String fred
      11: aload_0
      12: ldc           #5                  // String fred
      14: ldc           #6                  // class java/lang/Integer
      16: invokevirtual #7                  // Method get:(Ljava/lang/String;Ljava/lang/Class;)Ljava/lang/Object;
      19: invokeinterface #8,  3            // InterfaceMethod java/util/Map.put:(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;
      24: pop
      25: aload_1
      26: areturn

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:

User myUser = doc.get("user", User.getClass());
 
// Is the same as:
User myUser = (User) doc.get("user");

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!

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