How to handle a Findbugs "Non-transient non-serializable instance field in serializable class"?

Question

Consider the class below. If I run Findbugs against it it will give me an error ("Non-transient non-serializable instance field in serializable class") on line 5 but not on line 7.

1 public class TestClass implements Serializable {
2
3  private static final long serialVersionUID = 1905162041950251407L;
4
5  private Set<Integer> mySet;      // Findbugs error
6
7  private HashSet<Integer> myOtherSet;
8
9 }

That's correct because java.util.Set never implements Serializable in its hierarchy and java.util.HashSet does. However it is best practice to code against interfaces instead of concrete implementations.

How can I best handle this?

I can add a @Suppresswarnings(justification="No bug", values="SE_BAD_FIELD") on line 3. I have quite a lot of Sets and Lists in my actual code and I'm afraid it will litter my code too much.

Are there better ways?

Exordium

What should be done if we are getting above issue due to byte[] used in a Serialized class? – Navicert

I can't trigger this error currently on this Java code. Was findbugs behavior modified? – Pyrosis

Answer 1

However it is best practice to code against interfaces instead of concrete implementations.

I submit that no, in this case it is not. Findbugs quite correctly tells you that you risk running into a NotSerializableException as soon as you have a non-serializable Set implementation in that field. This is something you should deal with. How, that depends on the design of your classes.

• If those collections are initialized within the class and never set from outside, then I see absolutely nothing wrong with declaring the concrete type for the field, since fields are implementation details anyway. Do use the interface type in the public interface.
• If the collection are passed into the class via a public interface, you have to ensure that they are in fact Serializable. To do that, create an interface SerializableSet extends Set, Serializable and use it for your field. Then, either:
  • Use SerializableSet in the public interface and provide implementation classes that implement it.
  • Check collections passed to the class via instanceof Serializable and if they're not, copy them into something that is.

Georgiannageorgianne

I'm going to go with the concrete implementations. Thanks. – Exordium

ew. i dislike using a concrete type, even in this situation. i think this is a safely ignorable warning. the only part you might need to worry about is if you truly have arbitrary code setting this collection which might set it to a non-serializable set instance. – Whaling

I just noticed that when I have an abstract class, the bug isn't triggered. Why wouldn't it? – Exordium

@jtahlborn: which is exactly why it's not a safely ignorable warning, and why using a concrete type is the correct solution: either it's never set from outside and using the concrete type is not a problem, or it's set from outside and you'd have to redesign the class anyway to make sure it can't be set to a non-serializable type (). Concrete types are really only a problem when they appear in method/constructor signatures, not in fields or local variables. – Georgiannageorgianne

@Koohoolinn: which abstract class? – Georgiannageorgianne

@Michael using the concrete type internally may not be a "problem", but i think it's a bad practice. and even if it is set externally, you would only have to worry if you might be dealing with code outside of your control. i feel that in this situation, the cleanliness of the design (using the interface) outweighs the (theoretical) usefulness of this warning. – Whaling

@Michael Take my example from above and make the class abstract: no more Findbugs error! – Exordium

@Koohoolinn: Could be an oversight by Findbugs. At least I can't think of a good reason. – Georgiannageorgianne

@jtahlborn: without justification, "bad practice" and "clean design" are meaningless words. The possibility of getting a runtime exception after client code is change, no matter whether it's in- or outside your control, is far less theoretical than your usage of those words. In fact, I would say that using the concrete type in this case is good practice and definitely cleaner design. – Georgiannageorgianne

I agree with @Whaling . You can't make every method accept only HashSet when you really need a set. Callers shouldn't be required to pass an HashSet, any Serializable Set implementation would do. This is something you can't express in Java right now, a language design limitation you have to deal with. I think it's safer to use the interface and ignore this warning (or just check you're ok, without making it 100% sure). – Numerical

@ymajoros: There is no sensible dfinition of the word "safe" for which your statement would be true. Safety is in fact exactly the reason why using the concrete type is a much better solution than ignoring the warning. – Georgiannageorgianne

@Michael - the concrete type is no safer than an interface unless you are dealing with a "final" type. i can easily create a custom subclass of HashSet which is not serializable, so the compile check gains you nothing. if you are truly paranoid, then you should never set the collection directly from an outside instance, but always make a defensive copy. either way, my comments about using the interface are still valid: thus the warning is not helpful. – Whaling

@Michael: let's say I used "safer" as "the whole picture looks better". – Numerical

@Numerical my answer shows that it's possible in java to express those requirements. – Florrie

FindBugs warns even when the field is marked final and so it can prove that it is really a HashSet being stored. This just feels like a bug. – Serialize

"Check collections passed to the class via instanceof Serializable and throw an exception if they're not, otherwise cast them to SerializableSet". You can't cast HashSet to your custom interface. – Histrionics

@ChristianStrempfer: You're right, and it's amazing that nobody noticed that before. I think I mistakenly assumed that Java interface types with multiple inheritance could be used like set unions. I changed the answer to something less fancy that should work. – Georgiannageorgianne

Generics would make it possible to declare that a parameter must be Set & Serializable, but you should never use that trick. Mind that the standard API is full of collections not reporting their serializable nature at compile-time, Collections.emptySet(), Collections.singleton(…), Collections.unmodifiableSet(…), Collections.synchronizedSet(…), Java 9’s Set.of(…) implementations or the results of calling subMap(…, …) on a TreeMap. Generally, whenever the actual implementation class is not public. – Insanity

Answer 2

You can get rid of those Critical warning messages by adding the following methods to your class:

private void writeObject(ObjectOutputStream stream)
        throws IOException {
    stream.defaultWriteObject();
}

private void readObject(ObjectInputStream stream)
        throws IOException, ClassNotFoundException {
    stream.defaultReadObject();
}

Santiago

Excellent find! This resolves a really obscure findbugs error. Also avoids the need to mark transient, which doesn't even help, as Rodrigo pointed out. – Lustring

Work list a charm! Thanks! – Terzetto

Adding obsolete code to make an audit tool happy (obviously exploiting a bug in that tool) while the program behavior stays the same, just becomes slightly more inefficient. Great thing… – Insanity

If you measure it, you'll probably see a 10 or 100 nanosecond overhead. Did you get different results when you measured it? – Santiago

What about the new errors introduced such as needlessly implements what is default streaming behaviour? – Hague

Answer 3

I know this is an old question that's already answered but just so others know is that you can set the Set<Integer> field as transient if you have no interest in serializing that particular field which will fix your FindBugs error.

public class TestClass implements Serializable {

    private static final long serialVersionUID = 1905162041950251407L;
    private transient Set<Integer> mySet;

}

I prefer this method instead of forcing users of your API to cast to your concrete type, unless it's just internal, then Michael Borgwardt's answer makes more sense.

Exemplar

This just changes the error to "The field mySet is transient but isn't set by deserialization" – Holloway

Yeah, using transient just yields other findbugs error: SE_TRANSIENT_FIELD_NOT_RESTORED. Vlad's answer below fixed it for me. – Lustring

he is marking TestClass Serializable . He wants to serialize it along with fields. – Knout

FWIW, adding transient worked in my case. Maybe something has changed around this stuff since 2015. – Hyacinthie

Answer 4

You could use a capture helper to ensure that a passed in Set supports two interfaces:

private static class SerializableTestClass<T extends Set<?> & Serializable> implements Serializable
{
    private static final long serialVersionUID = 1L;
    private final T serializableSet;

    private SerializableTestClass(T serializableSet)
    {
        this.serializableSet = serializableSet;
    }
}

public static class PublicApiTestClass
{
    public static <T extends Set<?> & Serializable> Serializable forSerializableSet(T set)
    {
        return new SerializableTestClass<T>(set);
    }
}

In this way you can have a public API that enforces Serializable without checking/requiring specific implementation details.

Florrie

What should be done if we are getting above issue due to byte[] used in a Serialized class? – Navicert

Answer 5

I use a findbugs-exclude Filter for collection-Fields:

<Match>
    <Field type="java.util.Map" />
    <Bug pattern="SE_BAD_FIELD" />
</Match>
<Match>
    <Field type="java.util.Set" />
    <Bug pattern="SE_BAD_FIELD" />
</Match>
<Match>
    <Field type="java.util.List" />
    <Bug pattern="SE_BAD_FIELD" />
</Match>

See http://findbugs.sourceforge.net/manual/filter.html

Chanukah

It's not an error! All known Implementations are Serializable. Java doesn't support defining new Types like "type SerializableList = java.util.List & Serializable". And creating an Interface doesn't solve the Problem because e.g.: ArrayList is Serializable and a List but dosn't match your Interface. – Chanukah

@brabenetz what?? Was that a serious response? are you really meaning to tell me that you think you can't do interface Foo extends Serializable, List? Java most certainly does support defining new types...that's literally what Java is used for...that's the trademark of any OO language... – Shimmer

@Shimmer sure you can define your own Interface "Foo", but no List implementation from the JDK will implement your interface "Foo". You can NOT define an anonymous type in java (AFAIK) like "private Set<Integer> & Serializable mySet;" You can use Generics (as jontejj has described in https://mcmap.net/q/295920/-how-to-handle-a-findbugs-quot-non-transient-non-serializable-instance-field-in-serializable-class-quot ), but generics has its own problems and in this case makes your code difficult to read, write and use. – Chanukah

You aren't understanding. Just because you can't find a class like that in the JavaSE API, DOES NOT MEAN: 1) that it will never exist in the JavaSE API, 2) that JavaEE, JavaFX, JavaME, etc etc etc etc etc does not have such a collection 3) that your code is not using a third party library with such a collection, 4) that your own code does not have such a class defined. If you start just closing your eyes, as dounyy put it, then you are inevitably going to start missing some of the important things Findbugs is telling you. – Shimmer

Sure I understand, but I prioritize the Rule "Keep It Simple" higher than this rule. And at the end it depends on your Project. On an Open-Source-Project you may benefit from the additional complexity. But on a Closed-Source-Project you normally know who accesses your API (if it is even an API at all). – Chanukah

Often you only inherited the Serialization interface even if you not need it. E.g. how often have you serialized an Exception? Nowadays Objects are more often "Serialized" to JSON or XML and for this, the Serializable Interface is not needed. At the end it depends on YOUR code and usage. And a "Warning" means YOU must decide if it must be compile-save. – Chanukah

@Shimmer to shorten the discussion, the set returned by keySet() invoked on a HashMap does not implement Serializable. So yes, there are implementations which do not implement Serializable. On the other hand, the set return by subSet(x, y ) invoked on a TreeSet does implement Serializable, likewise do the other dozen sets and you will have a hard time convincing an audit tool like findbugs that this non-public type implements Serializable when you can’t access it at compile-time. – Insanity

Answer 6

Use a concrete Serializable set for your internal representation, but make any public interfaces use the Set interface.

public class TestClass implements Serializable {
    private static final long serialVersionUID = 1905162041950251407L;

    private HashSet<Integer> mySet;

    public TestClass(Set<Integer> s) {
        super();
        setMySet(s);
    }

    public void setMySet(Set<Integer> s) {
        mySet = (s == null) ? new HashSet<>() : new HashSet<>(s);
    }
}

Imbibition

Answer 7

I had HIGH Warning for a protected field in a serializable class. Add transient for the field resolved my problem:

 protected transient Object objectName;

Leishaleishmania

Answer 8

In case you are using findbugs-maven-plugin and have to persist a field, and that field is a class not implementing Serializable interface, for example, a field that has a class defined in a 3rd party. You can manually configure exclude file for findbugs,

If this is the only case, add it in an exclude file: pom:

<plugin>
    <groupId>org.codehaus.mojo</groupId>
    <artifactId>findbugs-maven-plugin</artifactId>
    <version>3.0.3</version>
    <configuration>
          <xmlOutput>true</xmlOutput>
          <xmlOutputDirectory>target/findbugs/</xmlOutputDirectory>
          <excludeFilterFile>findbugs-exclude.xml</excludeFilterFile>
          <includeFilterFile>findbugs-include.xml</includeFilterFile>
          <failOnError>true</failOnError>
    </configuration>
...

exclude.xml:

<?xml version="1.0" encoding="UTF-8"?>
<FindBugsFilter>
    <Match>
        <Class name="com.xxx.Foo" /> 
        <Field type="org.springframework.statemachine.StateMachineContext"/>
    </Match>

Entity:

@Entity
public class Foo extends Boo {
    StateMachineContext<A, B> stateMachineContext;

Although I don't understand why adding <Bug category="SE_BAD_FIELD"/> would not work. Besides, I don't agree with the solution of adding annotation on the field like @edu.umd.cs.findbugs.annotations.SuppressWarnings(justification="No bug", values="SE_BAD_FIELD"), because building tools better not penetrate business code.maven plugin usage & findbugs filters both include and exclude

About SE_BAD_FIELD: Non-transient non-serializable instance field in serializable class, I think it should not check on entities. Because, javax.persistence.AttributeConverter offers methods to serialize a field out side (implements Serializable is an inside method to serialize).

Kelsiekelso