Ensure all adapters and decorators can be extended #806
Conversation
victornoel
force-pushed
the
647-adapter-and-decorators-non-final
branch
from
aba4998
to
8edb4d8
Compare
Closes eclipse#647 Signed-off-by: Victor Noël <victor.noel@brennus-analytics.com>
victornoel
force-pushed
the
647-adapter-and-decorators-non-final
branch
from
8edb4d8
to
e41bf15
Compare
|
I believe this is technically a security vulnerability, but I'm also kind of open to it. A malicious user could deserialize some custom bytes into a subclass of an unmodifiable wrapper that's not actually unmodifiable. On the other hand, I haven't used serialization in years and if you're security conscious, you probably forbid deserialization. It's something to think about though. We prevented subclassing on purpose at the time. The JDK wrappers do the same thing. |
|
I'm looking around in Guava, and they don't even have concrete classes. Unmodifiable wrappers are all abstract. I don't fully understand why the concrete classes must have hidden visibility, but I think it's important. |
| @@ -44,7 +44,7 @@ | |||
| * {@link #wrapList(Iterable)} factory method. To wrap a collection with the MutableCollection interface alone, use | |||
| * the {@link #adapt(Collection)} factory method. | |||
| */ | |||
| public final class CollectionAdapter<T> | |||
| public class CollectionAdapter<T> | |||
| extends AbstractCollectionAdapter<T> | |||
There was a problem hiding this comment.
Note how CollectionAdapter extends AbstractCollectionAdapter. This is the pattern I think we should copy. The superclass is public api.
| @@ -64,7 +64,7 @@ | |||
| extends AbstractUnmodifiableMutableCollection<T> | |||
| implements MutableBag<T>, Serializable | |||
| { | |||
| UnmodifiableBag(MutableBag<? extends T> mutableBag) | |||
| protected UnmodifiableBag(MutableBag<? extends T> mutableBag) | |||
There was a problem hiding this comment.
Note how UnmodifiableBag extends AbstractUnmodifiableMutableCollection. I think the right thing to do here is extract a new superclass AbstractUnmodifiableBag, which becomes part of our public api. I think we should leave the sublcass alone.
There was a problem hiding this comment.
what would be left in the UnmodifiableBag class at then end? nothing except being final for security purpose?
There was a problem hiding this comment.
That's right. I'm not even sure I know why, but I see that the JDK does it and Guava does it, and I think we must have done it for a similar reason.
|
@motlin in my own projects, I usually use the following pattern to solve this kind of situation:
With this pattern, I can define any implementation of the interfaces in terms of existing implementations. See https://github.com/victornoel/eo-envelopes for a tooling made and use to simplify my life to apply this pattern. Writing envelopes for all the interface of EC could be a good alternative to this PR :) |
I got bitten by #647 again so decided to make a PR directly :)
The constructors could have been made public but the EC way seems to be using the
of()static method for this usage, so I left it at that.