fixing serialization bug with order of class properties (#1698)

tulinkry · Vladimir Kotal · commit ce4942f782cb · 2017-07-26T22:50:10.000+02:00
diff --git a/src/org/opensolaris/opengrok/configuration/Group.java b/src/org/opensolaris/opengrok/configuration/Group.java
@@ -27,6 +27,7 @@
 import java.util.TreeSet;
 import java.util.regex.Pattern;
 import java.util.regex.PatternSyntaxException;
+import org.opensolaris.opengrok.util.ClassUtil;
 
 /**
  * Placeholder for the information about subgroups of projects and repositories.
@@ -38,6 +39,10 @@
  */
 public class Group implements Comparable<Group>, Nameable {
 
+    static {
+        ClassUtil.remarkTransientFields(Group.class);
+    }
+
     private String name;
     /**
      * Group regexp pattern.
@@ -60,13 +65,13 @@ public class Group implements Comparable<Group>, Nameable {
      */
     private Pattern compiledPattern = Pattern.compile("()");
     private Group parent;
-    private int flag;
-
     private Set<Group> subgroups = new TreeSet<>();
-    private Set<Group> descendants = new TreeSet<>();
-    private Set<Project> projects = new TreeSet<>();
-    private Set<Project> repositories = new TreeSet<>();
-    private Set<Group> parents;
+
+    private transient int flag;
+    private transient Set<Group> descendants = new TreeSet<>();
+    private transient Set<Project> projects = new TreeSet<>();
+    private transient Set<Project> repositories = new TreeSet<>();
+    private transient Set<Group> parents;
 
     /**
      * No-arg constructor is needed for deserialization.
diff --git a/src/org/opensolaris/opengrok/configuration/Project.java b/src/org/opensolaris/opengrok/configuration/Project.java
@@ -29,6 +29,7 @@
 import java.util.Locale;
 import java.util.Set;
 import java.util.TreeSet;
+import org.opensolaris.opengrok.util.ClassUtil;
 
 /**
  * Placeholder for the information that builds up a project
@@ -37,6 +38,10 @@ public class Project implements Comparable<Project>, Nameable, Serializable {
 
     private static final long serialVersionUID = 1L;
 
+    static {
+        ClassUtil.remarkTransientFields(Project.class);
+    }
+
     private String path;
 
     /**
@@ -67,17 +72,17 @@ public class Project implements Comparable<Project>, Nameable, Serializable {
     /**
      * Set of groups which match this project.
      */
-    private Set<Group> groups = new TreeSet<>();
+    private transient Set<Group> groups = new TreeSet<>();
 
     // This empty constructor is needed for serialization.
-    public Project () {
+    public Project() {
     }
 
-    public Project (String name) {
+    public Project(String name) {
         this.name = name;
     }
 
-    public Project (String name, String path) {
+    public Project(String name, String path) {
         this.name = name;
         this.path = path;
     }
@@ -128,8 +133,8 @@ public int getTabSize() {
      * Set a textual name of this project, preferably don't use " , " in the
      * name, since it's used as delimiter for more projects
      *
-     * XXX we should not allow setting project name after it has been constructed
-     * because it is probably part of HashMap.
+     * XXX we should not allow setting project name after it has been
+     * constructed because it is probably part of HashMap.
      *
      * @param name a textual name of the project
      */
@@ -278,7 +283,7 @@ public static Project getByName(String name) {
         if (env.hasProjects()) {
             Project proj;
             if ((proj = env.getProjects().get(name)) != null) {
-                    return (proj);
+                return (proj);
             }
         }
         return null;
diff --git a/src/org/opensolaris/opengrok/util/ClassUtil.java b/src/org/opensolaris/opengrok/util/ClassUtil.java
@@ -0,0 +1,72 @@
+/*
+ * CDDL HEADER START
+ *
+ * The contents of this file are subject to the terms of the
+ * Common Development and Distribution License (the "License").
+ * You may not use this file except in compliance with the License.
+ *
+ * See LICENSE.txt included in this distribution for the specific
+ * language governing permissions and limitations under the License.
+ *
+ * When distributing Covered Code, include this CDDL HEADER in each
+ * file and include the License file at LICENSE.txt.
+ * If applicable, add the following below this CDDL HEADER, with the
+ * fields enclosed by brackets "[]" replaced with your own identifying
+ * information: Portions Copyright [yyyy] [name of copyright owner]
+ *
+ * CDDL HEADER END
+ */
+
+ /*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ */
+package org.opensolaris.opengrok.util;
+
+import java.beans.BeanInfo;
+import java.beans.IntrospectionException;
+import java.beans.Introspector;
+import java.beans.PropertyDescriptor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+import org.opensolaris.opengrok.configuration.Project;
+import org.opensolaris.opengrok.logger.LoggerFactory;
+
+/**
+ *
+ * @author Krystof Tulinger
+ */
+public class ClassUtil {
+
+    private static final Logger LOGGER = LoggerFactory.getLogger(ClassUtil.class);
+
+    /**
+     * Mark all transient fields in {@code targetClass} as @Transient for the
+     * XML serialization.
+     *
+     * Fields marked with java transient keyword do not work becase the
+     * XMLEncoder does not take these into account. This helper marks the fields
+     * marked with transient keyword as transient also for the XMLDecoder.
+     *
+     * @param targetClass the class
+     */
+    public static void remarkTransientFields(Class targetClass) {
+        try {
+            BeanInfo info;
+            info = Introspector.getBeanInfo(targetClass);
+            PropertyDescriptor[] propertyDescriptors = info.getPropertyDescriptors();
+            for (Field f : Project.class.getDeclaredFields()) {
+                if (Modifier.isTransient(f.getModifiers())) {
+                    for (int i = 0; i < propertyDescriptors.length; ++i) {
+                        if (propertyDescriptors[i].getName().equals(f.getName())) {
+                            propertyDescriptors[i].setValue("transient", Boolean.TRUE);
+                        }
+                    }
+                }
+            }
+        } catch (IntrospectionException ex) {
+            LOGGER.log(Level.WARNING, "An exception ocurred during remarking transient fields:", ex);
+        }
+    }
+}
diff --git a/test/org/opensolaris/opengrok/configuration/ConfigurationTest.java b/test/org/opensolaris/opengrok/configuration/ConfigurationTest.java
@@ -0,0 +1,111 @@
+/*
+ * CDDL HEADER START
+ *
+ * The contents of this file are subject to the terms of the
+ * Common Development and Distribution License (the "License").
+ * You may not use this file except in compliance with the License.
+ *
+ * See LICENSE.txt included in this distribution for the specific
+ * language governing permissions and limitations under the License.
+ *
+ * When distributing Covered Code, include this CDDL HEADER in each
+ * file and include the License file at LICENSE.txt.
+ * If applicable, add the following below this CDDL HEADER, with the
+ * fields enclosed by brackets "[]" replaced with your own identifying
+ * information: Portions Copyright [yyyy] [name of copyright owner]
+ *
+ * CDDL HEADER END
+ */
+
+ /*
+ * Copyright (c) 2017, Oracle and/or its affiliates. All rights reserved.
+ */
+package org.opensolaris.opengrok.configuration;
+
+import java.beans.XMLDecoder;
+import java.beans.XMLEncoder;
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.util.LinkedList;
+import junit.framework.AssertionFailedError;
+import org.junit.Test;
+import org.opensolaris.opengrok.util.ClassUtil;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+
+/**
+ *
+ * @author Krystof Tulinger
+ */
+public class ConfigurationTest {
+
+    /**
+     * Test for a serialization bug in configuration. The problem is that with
+     * this scenario the output is written in a way that when deserializing the
+     * input later on, we get {@link NullPointerException} trying to use
+     * {@link Group#compareTo(Group)}. This exception is caused by wrong order
+     * of serialization of
+     * {@link Group#getDescendants()}, {@link Group#getParent()} and
+     * {@link Project#getGroups()} where the backpointers in a {@link Project}
+     * to several {@link Group}s shall be stored in a set while this
+     * {@link Group} does not have a name yet (= {@code null}).
+     *
+     * @throws IOException
+     * @see ClassUtil#remarkTransientFields(java.lang.Class)
+     * ClassUtil#remarkTransientFields() for suggested solution
+     */
+    @Test
+    public void serializationOrderTest() throws IOException {
+        Project project = new Project("project");
+        Group apache = new Group("Apache", "test.*");
+        Group bsd = new Group("BSD", "test.*");
+        Group opensource = new Group("OpenSource", "test.*");
+
+        opensource.addGroup(apache);
+        opensource.addGroup(bsd);
+
+        bsd.addProject(project);
+        opensource.addProject(project);
+
+        project.getGroups().add(opensource);
+        project.getGroups().add(bsd);
+
+        Configuration cfg = new Configuration();
+        Configuration oldCfg = cfg;
+        cfg.addGroup(apache);
+        cfg.addGroup(bsd);
+        cfg.addGroup(opensource);
+
+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+
+        try (XMLEncoder enc = new XMLEncoder(out)) {
+            enc.writeObject(cfg);
+        }
+
+        // Create an exception listener to detect errors while encoding and
+        // decoding
+        final LinkedList<Exception> exceptions = new LinkedList<>();
+
+        try (ByteArrayInputStream in = new ByteArrayInputStream(out.toByteArray());
+                XMLDecoder dec = new XMLDecoder(in, null, (Exception e) -> {
+                    exceptions.addLast(e);
+                })) {
+
+            cfg = (Configuration) dec.readObject();
+            assertNotNull(cfg);
+
+            // verify that the read didn't fail
+            if (!exceptions.isEmpty()) {
+                AssertionFailedError afe = new AssertionFailedError(
+                        "Got " + exceptions.size() + " exception(s)");
+                // Can only chain one of the exceptions. Take the first one.
+                afe.initCause(exceptions.getFirst());
+                throw afe;
+            }
+
+            assertEquals(oldCfg.getGroups(), cfg.getGroups());
+        }
+    }
+}
