[GitHub] brooklyn-server pull request #718: improvement to OSGi serialization strateg...

[neykov <gi...@git.apache.org>]

Github user neykov commented on a diff in the pull request:

    https://github.com/apache/brooklyn-server/pull/718#discussion_r120658487
  
    --- Diff: core/src/main/java/org/apache/brooklyn/util/core/xstream/XmlSerializer.java ---
    @@ -89,33 +96,49 @@ protected MapperWrapper wrapMapper(MapperWrapper next) {
         }
     
         /**
    -     * JCC is used when class names are serialized/deserialized and no alias is defined;
    -     * it is configured in XStream *without* access to the XStream mapper.
    +     * JCC is used when Class instances are serialized/deserialized as a value 
    +     * (not as tags) and there are no aliases configured for that type.
    +     * It is configured in XStream default *without* access to the XStream mapper,
    +     * which is meant to apply when serializing the type name for instances of that type.
    +     * <p>
          * However we need a few selected mappers (see {@link #wrapMapperForAllLowLevelMentions(Mapper)} )
    -     * in order to effect renames at the low level, but many of the mappers must NOT be used,
    +     * to apply to all class renames, but many of the mappers must NOT be used,
          * e.g. because some might intercept all Class<? extends Entity> references
          * (and that interception is only wanted when serializing <i>instances</i>,
          * as in {@link #wrapMapperForNormalUsage(Mapper)}).
          * <p>
    -     * This can typically be done simply by registering our own instance (due to order guarantee of PrioritizedList),
    +     * This can typically be done simply by registering our own instance of this (due to order guarantee of PrioritizedList),
          * after the instance added by XStream.setupConverters()
          */
         private JavaClassConverter newCustomJavaClassConverter() {
             return new JavaClassConverter(wrapMapperForAllLowLevelMentions(new DefaultMapper(xstream.getClassLoaderReference()))) {};
         }
         
    -    /** Adds mappers needed for *any* reference to a class, e.g. when names are used for inner classes, or classes are renamed;
    -     * this *excludes* basic mentions, however, because most rewrites should *not* be applied at this deep level;
    -     * mappers which effect aliases or intercept references to entities are usually NOT be invoked in this low-level pathway.
    -     * See {@link #newCustomJavaClassConverter()}. */
    +    /** Adds mappers needed for *any* reference to a class, both "normal" usage (when xstream wants a mapper)
    +     * and Class conversion (when xstream needs to serialize an instance of Class and doesn't have an alias).
    +     * <p>
    +     * This should apply when nice names are used for inner classes, or classes are renamed;
    +     * however mappers which affect aliases or intercept references to entities are usually 
    +     * NOT be invoked in this low-level pathway. See {@link #newCustomJavaClassConverter()}. */
    +    // Developer note - this is called by the xstream subclass constructor in the constructor of this class,
    +    // so very few fields are populated
         protected MapperWrapper wrapMapperForAllLowLevelMentions(Mapper next) {
             MapperWrapper result = new CompilerIndependentOuterClassFieldMapper(next);
    +        
             Supplier<ClassLoader> classLoaderSupplier = new Supplier<ClassLoader>() {
                 @Override public ClassLoader get() {
                     return xstream.getClassLoaderReference().getReference();
                 }
             };
    -        return new ClassRenamingMapper(result, deserializingClassRenames, classLoaderSupplier);
    +        result = new ClassRenamingMapper(result, deserializingClassRenames, classLoaderSupplier);
    +        result = new OsgiClassnameMapper(new Supplier<XStream>() {
    +            @Override public XStream get() { return xstream; } }, result);
    --- End diff --
    
    Why a supplier if you are caching the value in the anonymous class? The xstream reference doesn't change, as the classloader one does, so could pass it directly.


---
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.
---