Enhance type safety when getting tag values #63
Conversation
This removes all uses of `ReadableHeader::getValue` which returns `Object` and replaces them with methods that return specific types. This simplifies the code and eliminates the need for the caller to cast the result. The method `RpmBaseTag::getDataType` was added to store the tag's data type. The tag's data type is checked when the various `ReadableHeader` methods are called.
| result.add(new Dependency(name, version, flagSet)); | ||
| } | ||
| return result; | ||
| final List<String> names = header.getStringList(namesTag); |
There was a problem hiding this comment.
I think we make some breaking changes on how this function behaves. Which is ok. However, I also think we should document that.
So far, to my understanding, the function simply ignored unexpected values and types. Now it will throw an exception. And while it's a private method, I think it makese sense adding a note and tracing calls that lead to this call and put a note on the API.
There was a problem hiding this comment.
Yes, the change is to throw the IllegalArgumentException instead of ignoring it. You mean to add javadoc for it (although it's an unchecked exception)?
There was a problem hiding this comment.
Is the current javadoc acceptable? I put it in the main text now as @throws because it's unchecked, but I wasn't sure about that.
| @@ -89,84 +68,60 @@ public int getIndex() { | |||
| return this.index; | |||
| } | |||
|
|
|||
| void fillFromStore(final ByteBuffer storeData) throws IOException { | |||
| @SuppressWarnings("unchecked") | |||
There was a problem hiding this comment.
I don't think the idea of using <T> here works out. Due to the type erasure which will happen, this will be the same code as code (Object). And the cast of RpmTagValue<T> will never trigger any error, it's the same as RpmTagValue<?> IIRC.
There was a problem hiding this comment.
OK, you're right, nothing is really gained by using the <E> Something<E> pattern over Something<?>, but I am not sure what the difference is.
There was a problem hiding this comment.
I think using Something<?> clearly indicates that this is the case. Where Something<E> gives a false impression. Also, it might lead to class cast exceptions:
public class Main {
static <E> E convert(Object value) {
return (E) value;
}
public static void main(String[] args) {
boolean value = true;
String converted = convert(value);
}
}The code will compile (with a warning the we suppress for RpmTagValue). However running the code will result in a ClassCastException. So it doesn't really add type safety, but creates a confusing API.
To my my understanding, in our case this might lead to a call where the user expects to get a RpmTagValue<String>, but indeed it would be RpmTagValue<Object>, and the content might be Boolean. As some point, assigning that to a String, it would trigger a ClassCastException.
There was a problem hiding this comment.
Here's an example from the Android SDK, where they did the same, and fixed it later: https://developer.android.com/reference/android/content/Intent#getParcelableArrayListExtra(java.lang.String). The ArrayList<T> might have contained all kinds of objects, not just T. Simply because that information (of T) is not available during runtime.
They replaced it with a method which received Class<T>, and so it is possible (during runtime) to .cast() to T, as the class information is present.
Having a background in C++ and working with Rust, this all seems weird :) I personally think type erasure in Java was a big mistake.
There was a problem hiding this comment.
Yes, I was trying to use this pattern of passing the class for the getOptionalTag/getEntry API.
| } | ||
|
|
||
| public Object getTagOrDefault(final T tag, final Object defaultValue) { | ||
| return getOptionalTag(tag).orElse(defaultValue); | ||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Same for the next method, I think we should add an actual type check. IIRC, you can use dataType.cast(..) for this. However IIRC using (HeaderValue<E>) won't work, because that's simply being erased and becomes HeaderValue.
There was a problem hiding this comment.
OK, I will try to use .cast() here.
There was a problem hiding this comment.
Actually, now we're just returning HeaderValue with no type, so there's nothing to cast, right?
There was a problem hiding this comment.
There's still some inconsistency in my current code which still has:
public <E> Optional<HeaderValue> getOptionalTag(final T tag, Class<E> dataType);
private Optional<HeaderValue> getEntry(final int tag, Class<?> dataType);Are you sure nothing is gained by HeaderValue<?>, then we'd have the type <E>, which says that the type in header value should match the type in the class argument.
<E> Optional<HeaderValue<E>> getOptionalTag(final T tag, Class<E> dataType);There was a problem hiding this comment.
Using:
<E> Optional<HeaderValue<E>> getOptionalTag(final T tag, Class<E> dataType);would actually have a benefit if the type of the value really is being limited to E. That requires casting using dataType.cast(..). It would also be required to handle the case when the value is not of type E. Throw an exception, return None would both work.
The question is just if that makes the code calling into it much better?
Given Java's new pattern type construct (not sure I got the name right) one should be able to do something like:
if (headers.getEntry(Tags.MyTag) instanceof String s) {
// do something with s
}When reading, the header can basically carry anything, and we can't control that. Only when creating a header, we could enforce (via the type system) that we user cannot create wrong headers.
This removes all uses of
ReadableHeader::getValuewhich returnsObjectand replaces them with methods that return specific types. This simplifies the code and eliminates the need for the caller to cast the result.The method
RpmBaseTag::getDataTypewas added to store the tag's data type. The tag's data type is checked when the variousReadableHeadermethods are called.