brooklyn-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From neykov <...@git.apache.org>
Subject [GitHub] brooklyn-server pull request #598: BROOKLYN-453: Fix rebind for classrename ...
Date Thu, 16 Mar 2017 09:25:20 GMT
Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/598#discussion_r106369844
  
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/ClassRenamingMapper.java
---
    @@ -22,32 +22,168 @@
     
     import java.util.Map;
     
    +import org.apache.brooklyn.core.mgmt.persist.OsgiClassPrefixer;
     import org.apache.brooklyn.util.guava.Maybe;
     import org.apache.brooklyn.util.javalang.Reflections;
     import org.slf4j.Logger;
     import org.slf4j.LoggerFactory;
     
    +import com.google.common.base.Supplier;
    +import com.thoughtworks.xstream.mapper.CannotResolveClassException;
     import com.thoughtworks.xstream.mapper.Mapper;
     import com.thoughtworks.xstream.mapper.MapperWrapper;
     
    +/**
    + * An xstream mapper that handles class-renames, so we can rebind to historic persisted
state.
    + */
     public class ClassRenamingMapper extends MapperWrapper {
    +    
    +    /*
    +     * TODO There is a strange relationship between this and XmlMementoSerializer$OsgiClassnameMapper.
    +     * Should these be perhaps merged?
    +     * 
    +     * TODO For class-loading on deserialzation, should we push the class-rename logic
into 
    +     * org.apache.brooklyn.util.core.ClassLoaderUtils instead? Does the xstream mapper
do
    +     * anything else important, beyond that class-loading responsibility? It's registration
    +     * in XmlSerializer makes it look a bit scary: wrapMapperForAllLowLevelMentions().
    +     * 
    +     * ---
    +     * TODO This code feels overly complicated, and deserves a cleanup.
    +     * 
    +     * The aim is to handle two use-cases in the deserializingClassRenames.properties:
    +     * 
    +     *  1. A very explicit rename that includes bundle prefixes (e.g. so as to limit
scope, or to support 
    +     *     moving a class from one bundle to another).
    +     *  
    +     *  2. Just the class-rename (e.g. `com.acme.Foo: com.acme.Bar`).
    +     *     This would rename "acme-bundle:com.acme.Foo" to "acme-bundle:com.acme.Bar".
    +     * 
    +     * However, to achieve that is fiddly for several reasons:
    +     * 
    +     *  1. We might be passed qualified or unqualified names (e.g. "com.acme.Foo" or
"acme-bundle:com.acme.Foo"),
    +     *     depending how old the persisted state is, where OSGi was used previously,
and whether 
    +     *     whitelabelled bundles were used. 
    +     * 
    +     *  2. Calling `super.realClass(name)` must return a class that has exactly the same
name as 
    +     *     was passed in. This is because xstream will subsequently use `Class.forName`
which is 
    +     *     fussy about that. However, if we're passed "acme-bundle:com.acme.Foo" then
we'd expect
    +     *     to return a class named "com.acme.Foo". The final classloading in our 
    +     *     `XmlMementoSerializer$OsgiClassLoader.findClass()` will handle stripping out
the bundle
    +     *     name, and using the right bundle.
    +     *     
    +     *     In the case where we haven't changed the name, then we can leave it up to

    +     *     `XmlMementoSerializer$OsgiClassnameMapper.realClass()` to do sort this out.
But if we've 
    +     *     done a rename, then unforutnately it's currently this class' responsibility!
    +     *     
    +     *     That means it has to fallback to calling classLoader.loadClass().
    +     *  
    +     *  3. As mentioned under the use-cases, the rename could include the full bundle
name prefix, 
    +     *     or it might just be the classname. We want to handle both, so need to implement
yet
    +     *     more fallback behaviour.
    +     * 
    +     * ---
    +     * TODO Wanted to pass xstream, rather than Supplier<ClassLoader>, in constructor.
However, 
    +     * this caused NPE because of how this is constructed from inside 
    +     * XmlMementoSerializer.wrapMapperForNormalUsage, called from within an anonymous
subclass of XStream!
    +     */
    +    
         public static final Logger LOG = LoggerFactory.getLogger(ClassRenamingMapper.class);
         
         private final Map<String, String> nameToType;
    -
    -    public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType)
{
    +    private final Supplier<? extends ClassLoader> classLoaderSupplier;
    +    
    +    public ClassRenamingMapper(Mapper wrapped, Map<String, String> nameToType,
Supplier<? extends ClassLoader> classLoaderSupplier) {
             super(wrapped);
             this.nameToType = checkNotNull(nameToType, "nameToType");
    +        this.classLoaderSupplier = checkNotNull(classLoaderSupplier, "classLoaderSupplier");
         }
         
         @Override
         public Class<?> realClass(String elementName) {
    +        String elementNamOrig = elementName;
    --- End diff --
    
    Typo elementNam**e**Orig


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message