Hi Sean,
On 9/26/2019 12:46 PM, Sean Mullan wrote:
On 9/23/19 4:16 PM, Joe Darcy wrote:
Hi Sean,
On 9/23/2019 12:19 PM, Sean Mullan wrote:
Hi Joe,
It's a little odd to suppress the warnings in the X509CertImpl class
since it is a subclass of java.security.cert.Certificate which
implements the writeReplace method so these fields are not serialized.
Also for other classes like X509Key which are internal it is a
little odd to suppress the warnings for fields like bitStringKey
that are not Serializable and are never serialized. It is probably
better to mark them as transient, but I'm not really sure it is
worth making those changes for otherwise stable code. I guess when I
look at some of the warnings, I might think there is an issue when
there really isn't.
I suppose these are not things you can easily detect at compile
time, but I am wondering what you think.
Thanks for the feedback. One motivation to send out these review is
to help tune-up the warning analysis.
A brief philosophical note, when designing any sort of warning scheme
the cost of false positives vs false negatives needs to be weighed as
any static check can likely only approximate the full range of
dynamic behavior. The checks as written give up, i.e. do not attempt
to analyze and warn, if the class uses serialPersistentFields. It
would be possible for the checks to similarly decline to analyze if
any of the various write* or read* serialization methods are defined.
My current impression is that the net benefit for the checks when
write* methods are defined is still worthwhile, even though there are
some false positives.
As you observe, given the handling of the serial methods,
bitStringKey in X509Key is effectively transient. I think it would be
better serial coding for the field to be explicitly marked as
transient to make it more obviously consistent with the
implementation. Moreover, all the instance fields in X509Key except
encodedKey look to be effectively transient. Since the class already
has a serialVersionUID, marking the fields as transient is a
serial-compatible change.
Ok. Well I understand that the benefit here is to try to detect
serialization issues, and that is great. But at least in some cases
like in security libs where classes are doing their own alternate
serialization and have more than a few fields, the workaround of
leaving the warnings in or having to mark all the fields as transient
- either approach is not ideal to me. Could the
SuppressWarnings("serial") annotation be applied at the class level so
that it basically said "I know what I'm doing here, don't warn me
about serialization issues"?
Yes; applying SuppressWarnings("serial") to the class will silence this
new warning for all the fields. I didn't take that approach in the
version I sent to review since, by default, I wanted to provide the
narrowest scope of the suppression.
Would you prefer I revise the patch where there are multiple
SuppressWarnings("serial") on fields to put a single one on the class
instead?
Cheers,
-Joe
