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.
rpm/src/main/java/org/eclipse/packager/rpm/parse/HeaderValue.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| 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.
There was a problem hiding this comment.
would actually have a benefit if the type of the value really is being limited to
E. That requires casting usingdataType.cast(..). It would also be required to handle the case when the value is not of typeE. Throw an exception, returnNonewould both work.
My thoughts:
Adding a type parameter, say <E>, only seems to change two methods, getEntry and getOptionalTag, to take a Class<E> parameter. For the datatype.cast(), this doesn't work because the datatype is E, but we need HeaderValue<E>?
Also, now we're adding a type parameter to HeaderValue which never gets used by the class itself. This is different from adding it to RpmTagValue, where it is actually used.
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.