ATLAS-476 Update type attribute with Reserved characters updated the original…

release-log.txt

@@ -10,6 +10,7 @@ ATLAS-409 Atlas will not import avro tables with schema read from a file (dosset
 ATLAS-379 Create sqoop and falcon metadata addons (venkatnrangan,bvellanki,sowmyaramesh via shwethags)
 
 ALL CHANGES:
+ATLAS-476 Update type attribute with Reserved characters updated the original type as unknown (yhemanth via shwethags)
 ATLAS-463 Disconnect inverse references ( dkantor via sumasai)
 ATLAS-479 Add description for different types during create time (guptaneeru via shwethags)
 ATLAS-508 Apache nightly build failure - UnsupportedOperationException: Not a single key: __traitNames (shwethags)

repository/src/main/java/org/apache/atlas/repository/typestore/GraphBackedTypeStore.java

@@ -70,11 +70,6 @@ public class GraphBackedTypeStore implements ITypeStore {
     }
 
     @Override
-    public void store(TypeSystem typeSystem) throws AtlasException {
-        store(typeSystem, ImmutableList.copyOf(typeSystem.getTypeNames()));
-    }
-
-    @Override
     @GraphTransaction
     public void store(TypeSystem typeSystem, ImmutableList<String> typeNames) throws AtlasException {
         for (String typeName : typeNames) {
@@ -133,12 +128,9 @@ public class GraphBackedTypeStore implements ITypeStore {
     }
 
     private void storeInGraph(TypeSystem typeSystem, DataTypes.TypeCategory category, String typeName,
-        ImmutableList<AttributeInfo> attributes, ImmutableList<String> superTypes) throws AtlasException {
-        storeInGraph(typeSystem, category, typeName, null, attributes, superTypes);
-    }
-
-    private void storeInGraph(TypeSystem typeSystem, DataTypes.TypeCategory category, String typeName, String typeDescription,
-            ImmutableList<AttributeInfo> attributes, ImmutableList<String> superTypes) throws AtlasException {
+                              String typeDescription, ImmutableList<AttributeInfo> attributes,
+                              ImmutableList<String> superTypes)
+            throws AtlasException {
         Vertex vertex = createVertex(category, typeName, typeDescription);
         List<String> attrNames = new ArrayList<>();
         if (attributes != null) {
@@ -165,7 +157,6 @@ public class GraphBackedTypeStore implements ITypeStore {
         }
     }
 
-    //Add edges for complex attributes
     private void addReferencesForAttribute(TypeSystem typeSystem, Vertex vertex, AttributeInfo attribute)
             throws AtlasException {
         ImmutableList<String> coreTypes = typeSystem.getCoreTypes();

repository/src/main/java/org/apache/atlas/repository/typestore/ITypeStore.java

@@ -24,18 +24,13 @@ import org.apache.atlas.typesystem.TypesDef;
 import org.apache.atlas.typesystem.types.TypeSystem;
 
 public interface ITypeStore {
-    /**
-     * Persist the entire type system - insert or update
-     * @param typeSystem type system to persist
-     * @throws StorageException
-     */
-    void store(TypeSystem typeSystem) throws AtlasException;
 
     /**
-     * Persist the given type in the type system - insert or update
-     * @param typeSystem type system
-     * @param types types to persist
-     * @throws StorageException
+     * Add types to the underlying type storage layer
+     * @param typeSystem {@link TypeSystem} object which contains existing types. To lookup newly added types,
+     *                                     an instance of {@link TypeSystem.TransientTypeSystem} can be passed.
+     * @param types names of newly added types.
+     * @throws AtlasException
      */
     void store(TypeSystem typeSystem, ImmutableList<String> types) throws AtlasException;
 

repository/src/main/java/org/apache/atlas/services/DefaultMetadataService.java

@@ -169,20 +169,28 @@ public class DefaultMetadataService implements MetadataService {
      */
     @Override
     public JSONObject createType(String typeDefinition) throws AtlasException {
+        return createOrUpdateTypes(typeDefinition, false);
+    }
+
+    private JSONObject createOrUpdateTypes(String typeDefinition, boolean isUpdate) throws AtlasException {
         ParamChecker.notEmpty(typeDefinition, "type definition cannot be empty");
         TypesDef typesDef = validateTypeDefinition(typeDefinition);
 
         try {
-            final Map<String, IDataType> typesAdded = typeSystem.defineTypes(typesDef);
-
+            final TypeSystem.TransientTypeSystem transientTypeSystem = typeSystem.createTransientTypeSystem(typesDef, isUpdate);
+            final Map<String, IDataType> typesAdded = transientTypeSystem.getTypesAdded();
             try {
                 /* Create indexes first so that if index creation fails then we rollback
                    the typesystem and also do not persist the graph
                  */
-                onTypesAdded(typesAdded);
-                typeStore.store(typeSystem, ImmutableList.copyOf(typesAdded.keySet()));
+                if (isUpdate) {
+                    onTypesUpdated(typesAdded);
+                } else {
+                    onTypesAdded(typesAdded);
+                }
+                typeStore.store(transientTypeSystem, ImmutableList.copyOf(typesAdded.keySet()));
+                typeSystem.commitTypes(typesAdded);
             } catch (Throwable t) {
-                typeSystem.removeTypes(typesAdded.keySet());
                 throw new AtlasException("Unable to persist types ", t);
             }
 
@@ -197,30 +205,7 @@ public class DefaultMetadataService implements MetadataService {
 
     @Override
     public JSONObject updateType(String typeDefinition) throws AtlasException {
-        ParamChecker.notEmpty(typeDefinition, "type definition cannot be empty");
-        TypesDef typesDef = validateTypeDefinition(typeDefinition);
-
-        try {
-            final Map<String, IDataType> typesAdded = typeSystem.updateTypes(typesDef);
-
-            try {
-                /* Create indexes first so that if index creation fails then we rollback
-                   the typesystem and also do not persist the graph
-                 */
-                onTypesUpdated(typesAdded);
-                typeStore.store(typeSystem, ImmutableList.copyOf(typesAdded.keySet()));
-            } catch (Throwable t) {
-                typeSystem.removeTypes(typesAdded.keySet());
-                throw new AtlasException("Unable to persist types ", t);
-            }
-
-            return new JSONObject() {{
-                put(AtlasClient.TYPES, typesAdded.keySet());
-            }};
-        } catch (JSONException e) {
-            LOG.error("Unable to create response for types={}", typeDefinition, e);
-            throw new AtlasException("Unable to create response ", e);
-        }
+        return createOrUpdateTypes(typeDefinition, true);
     }
 
     private TypesDef validateTypeDefinition(String typeDefinition) {

repository/src/test/java/org/apache/atlas/repository/typestore/GraphBackedTypeStoreTest.java

@@ -38,6 +38,7 @@ import org.apache.atlas.typesystem.types.EnumType;
 import org.apache.atlas.typesystem.types.EnumTypeDefinition;
 import org.apache.atlas.typesystem.types.EnumValue;
 import org.apache.atlas.typesystem.types.HierarchicalTypeDefinition;
+import org.apache.atlas.typesystem.types.IDataType;
 import org.apache.atlas.typesystem.types.Multiplicity;
 import org.apache.atlas.typesystem.types.StructType;
 import org.apache.atlas.typesystem.types.StructTypeDefinition;
@@ -51,8 +52,8 @@ import org.testng.annotations.Guice;
 import org.testng.annotations.Test;
 
 import javax.inject.Inject;
-
 import java.util.List;
+import java.util.Map;
 
 import static org.apache.atlas.typesystem.types.utils.TypesUtil.createClassTypeDef;
 import static org.apache.atlas.typesystem.types.utils.TypesUtil.createOptionalAttrDef;
@@ -89,7 +90,8 @@ public class GraphBackedTypeStoreTest {
 
     @Test
     public void testStore() throws AtlasException {
-        typeStore.store(ts);
+        ImmutableList<String> typeNames = ts.getTypeNames();
+        typeStore.store(ts, typeNames);
         dumpGraph();
     }
 
@@ -177,9 +179,8 @@ public class GraphBackedTypeStoreTest {
                 ImmutableList.<HierarchicalTypeDefinition<TraitType>>of(),
                 ImmutableList.of(deptTypeDef, superTypeDef));
 
-        ts.updateTypes(typesDef);
-        typeStore.store(ts, ImmutableList.of(orgLevelEnum.name, addressDetails.typeName, superTypeDef.typeName,
-                deptTypeDef.typeName));
+        Map<String, IDataType> typesAdded = ts.updateTypes(typesDef);
+        typeStore.store(ts, ImmutableList.copyOf(typesAdded.keySet()));
 
         //Validate the updated types
         TypesDef types = typeStore.restore();

repository/src/test/java/org/apache/atlas/service/DefaultMetadataServiceTest.java

@@ -26,6 +26,10 @@ import com.thinkaurelius.titan.core.util.TitanCleanup;
 import org.apache.atlas.AtlasClient;
 import org.apache.atlas.typesystem.exception.TypeNotFoundException;
 import org.apache.atlas.typesystem.exception.EntityNotFoundException;
+import org.apache.atlas.typesystem.types.ClassType;
+import org.apache.atlas.typesystem.types.DataTypes;
+import org.apache.atlas.typesystem.types.HierarchicalTypeDefinition;
+import org.apache.atlas.typesystem.types.utils.TypesUtil;
 import org.apache.atlas.utils.ParamChecker;
 import org.apache.atlas.AtlasException;
 import org.apache.atlas.RepositoryMetadataModule;
@@ -47,6 +51,8 @@ import org.apache.atlas.typesystem.types.TypeSystem;
 import org.apache.atlas.typesystem.types.ValueConversionException;
 import org.apache.commons.lang.RandomStringUtils;
 import org.codehaus.jettison.json.JSONArray;
+import org.codehaus.jettison.json.JSONException;
+import org.codehaus.jettison.json.JSONObject;
 import org.testng.Assert;
 import org.testng.annotations.AfterTest;
 import org.testng.annotations.BeforeTest;
@@ -791,7 +797,33 @@ public class DefaultMetadataServiceTest {
         Assert.assertEquals(deletedGuidsFromListener.size(), deletedGuids.size());
         Assert.assertTrue(deletedGuidsFromListener.containsAll(deletedGuids));
     }
-    
+
+    @Test
+    public void testTypeUpdateWithReservedAttributes() throws AtlasException, JSONException {
+        String typeName = "test_type_"+ RandomStringUtils.randomAlphanumeric(10);
+        HierarchicalTypeDefinition<ClassType> typeDef = TypesUtil.createClassTypeDef(
+                typeName, ImmutableList.<String>of(),
+                TypesUtil.createUniqueRequiredAttrDef("test_type_attribute", DataTypes.STRING_TYPE));
+        TypesDef typesDef = new TypesDef(typeDef, false);
+        JSONObject type = metadataService.createType(TypesSerialization.toJson(typesDef));
+        Assert.assertNotNull(type.get(AtlasClient.TYPES));
+
+        HierarchicalTypeDefinition<ClassType> updatedTypeDef = TypesUtil.createClassTypeDef(
+            typeName, ImmutableList.<String>of(),
+            TypesUtil.createUniqueRequiredAttrDef("test_type_attribute", DataTypes.STRING_TYPE),
+            TypesUtil.createOptionalAttrDef("test_type_invalid_attribute$", DataTypes.STRING_TYPE));
+        TypesDef updatedTypesDef = new TypesDef(updatedTypeDef, false);
+
+        try {
+            metadataService.updateType(TypesSerialization.toJson(updatedTypesDef));
+            Assert.fail("Should not be able to update type with reserved character");
+        } catch (AtlasException ae) {
+            // pass.. expected
+        }
+        String typeDefinition = metadataService.getTypeDefinition(typeName);
+        Assert.assertNotNull(typeDefinition);
+    }
+
     private static class DeleteEntitiesChangeListener implements EntityChangeListener {
         
         private Collection<ITypedReferenceableInstance> deletedEntities_;

typesystem/src/main/java/org/apache/atlas/typesystem/types/TypeSystem.java

@@ -31,7 +31,6 @@ import javax.inject.Singleton;
 import java.lang.reflect.Constructor;
 import java.text.SimpleDateFormat;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
@@ -218,7 +217,7 @@ public class TypeSystem {
                 new TransientTypeSystem(ImmutableList.<EnumTypeDefinition>of(),
                         ImmutableList.<StructTypeDefinition>of(), ImmutableList.copyOf(traitDefs),
                         ImmutableList.<HierarchicalTypeDefinition<ClassType>>of());
-        return transientTypes.defineTypes();
+        return transientTypes.defineTypes(false);
     }
 
     public Map<String, IDataType> defineClassTypes(HierarchicalTypeDefinition<ClassType>... classDefs)
@@ -226,7 +225,7 @@ public class TypeSystem {
         TransientTypeSystem transientTypes = new TransientTypeSystem(ImmutableList.<EnumTypeDefinition>of(),
                 ImmutableList.<StructTypeDefinition>of(), ImmutableList.<HierarchicalTypeDefinition<TraitType>>of(),
                 ImmutableList.copyOf(classDefs));
-        return transientTypes.defineTypes();
+        return transientTypes.defineTypes(false);
     }
 
     public Map<String, IDataType> updateTypes(TypesDef typesDef) throws AtlasException {
@@ -257,7 +256,7 @@ public class TypeSystem {
             ImmutableList<HierarchicalTypeDefinition<TraitType>> traitDefs,
             ImmutableList<HierarchicalTypeDefinition<ClassType>> classDefs) throws AtlasException {
         TransientTypeSystem transientTypes = new TransientTypeSystem(enumDefs, structDefs, traitDefs, classDefs);
-        return transientTypes.defineTypes();
+        return transientTypes.defineTypes(false);
     }
 
     public DataTypes.ArrayType defineArrayType(IDataType elemType) throws AtlasException {
@@ -301,16 +300,44 @@ public class TypeSystem {
         return false;
     }
 
-    public void removeTypes(Collection<String> typeNames) {
-        for (String typeName : typeNames) {
-            IDataType dataType = types.get(typeName);
-            final DataTypes.TypeCategory typeCategory = dataType.getTypeCategory();
-            typeCategoriesToTypeNamesMap.get(typeCategory).remove(typeName);
-            types.remove(typeName);
+    /**
+     * Create an instance of {@link TransientTypeSystem} with the types defined in the {@link TypesDef}.
+     *
+     * As part of this, a set of verifications are run on the types defined.
+     * @param typesDef The new list of types to be created or updated.
+     * @param isUpdate True, if types are updated, false otherwise.
+     * @return {@link TransientTypeSystem} that holds the newly added types.
+     * @throws AtlasException
+     */
+    public TransientTypeSystem createTransientTypeSystem(TypesDef typesDef, boolean isUpdate) throws AtlasException {
+        ImmutableList<EnumTypeDefinition> enumDefs = ImmutableList.copyOf(typesDef.enumTypesAsJavaList());
+        ImmutableList<StructTypeDefinition> structDefs = ImmutableList.copyOf(typesDef.structTypesAsJavaList());
+        ImmutableList<HierarchicalTypeDefinition<TraitType>> traitDefs =
+                ImmutableList.copyOf(typesDef.traitTypesAsJavaList());
+        ImmutableList<HierarchicalTypeDefinition<ClassType>> classDefs =
+                ImmutableList.copyOf(typesDef.classTypesAsJavaList());
+        TransientTypeSystem transientTypeSystem = new TransientTypeSystem(enumDefs, structDefs, traitDefs, classDefs);
+        transientTypeSystem.verifyTypes(isUpdate);
+        return transientTypeSystem;
+    }
+
+    /**
+     * Commit the given types to this {@link TypeSystem} instance.
+     *
+     * This step should be called only after the types have been committed to the backend stores successfully.
+     * @param typesAdded newly added types.
+     */
+    public void commitTypes(Map<String, IDataType> typesAdded) {
+        for (Map.Entry<String, IDataType> typeEntry : typesAdded.entrySet()) {
+            String typeName = typeEntry.getKey();
+            IDataType type = typeEntry.getValue();
+            //Add/replace the new type in the typesystem
+            types.put(typeName, type);
+            typeCategoriesToTypeNamesMap.put(type.getTypeCategory(), typeName);
         }
     }
 
-    class TransientTypeSystem extends TypeSystem {
+    public class TransientTypeSystem extends TypeSystem {
 
         final ImmutableList<StructTypeDefinition> structDefs;
         final ImmutableList<HierarchicalTypeDefinition<TraitType>> traitDefs;
@@ -350,7 +377,7 @@ public class TypeSystem {
          * - validate cannot redefine types
          * - setup shallow Type instances to facilitate recursive type graphs
          */
-        private void step1(boolean update) throws AtlasException {
+        private void validateAndSetupShallowTypes(boolean update) throws AtlasException {
             for (EnumTypeDefinition eDef : enumDefs) {
                 assert eDef.name != null;
                 if (!update && (transientTypes.containsKey(eDef.name) || types.containsKey(eDef.name))) {
@@ -430,7 +457,7 @@ public class TypeSystem {
          * - for Hierarchical Types, validate SuperTypes.
          * - for each Hierarchical Type setup their SuperTypes Graph
          */
-        private void step2() throws AtlasException {
+        private void validateAndSetupSuperTypes() throws AtlasException {
             for (HierarchicalTypeDefinition<TraitType> traitDef : traitDefs) {
                 validateSuperTypes(TraitType.class, traitDef);
                 TraitType traitType = getDataType(TraitType.class, traitDef.typeName);
@@ -509,7 +536,7 @@ public class TypeSystem {
          * - Order Hierarchical Types in order of SuperType before SubType.
          * - Construct all the Types
          */
-        private void step3() throws AtlasException {
+        private void orderAndConstructTypes() throws AtlasException {
 
             List<TraitType> traitTypes = new ArrayList<>();
             for (String traitTypeName : traitNameToDefMap.keySet()) {
@@ -540,7 +567,7 @@ public class TypeSystem {
          * Step 4:
          * - fix up references in recursive AttrInfo and recursive Collection Types.
          */
-        private void step4() throws AtlasException {
+        private void setupRecursiveTypes() throws AtlasException {
             for (AttributeInfo info : recursiveRefs) {
                 info.setDataType(dataType(info.dataType().getName()));
             }
@@ -557,7 +584,7 @@ public class TypeSystem {
          * Step 5:
          * - Validate that the update can be done
          */
-        private void step5() throws TypeUpdateException {
+        private void validateUpdateIsPossible() throws TypeUpdateException {
             //If the type is modified, validate that update can be done
             for (IDataType newType : transientTypes.values()) {
                 if (TypeSystem.this.types.containsKey(newType.getName())) {
@@ -567,34 +594,11 @@ public class TypeSystem {
             }
         }
 
-        Map<String, IDataType> defineTypes() throws AtlasException {
-            return defineTypes(false);
-        }
-
         Map<String, IDataType> defineTypes(boolean update) throws AtlasException {
-            step1(update);
-            step2();
-
-            step3();
-            step4();
-
-            if (update) {
-                step5();
-            }
-
-            Map<String, IDataType> newTypes = new HashMap<>();
-
-            for (Map.Entry<String, IDataType> typeEntry : transientTypes.entrySet()) {
-                String typeName = typeEntry.getKey();
-                IDataType type = typeEntry.getValue();
-
-                //Add/replace the new type in the typesystem
-                TypeSystem.this.types.put(typeName, type);
-                typeCategoriesToTypeNamesMap.put(type.getTypeCategory(), typeName);
-
-                newTypes.put(typeName, type);
-            }
-            return newTypes;
+            verifyTypes(update);
+            Map<String, IDataType> typesAdded = getTypesAdded();
+            commitTypes(typesAdded);
+            return typesAdded;
         }
 
         @Override
@@ -672,6 +676,25 @@ public class TypeSystem {
         public DataTypes.MapType defineMapType(IDataType keyType, IDataType valueType) throws AtlasException {
             return super.defineMapType(keyType, valueType);
         }
+
+        void verifyTypes(boolean isUpdate) throws AtlasException {
+            validateAndSetupShallowTypes(isUpdate);
+            validateAndSetupSuperTypes();
+            orderAndConstructTypes();
+            setupRecursiveTypes();
+            if (isUpdate) {
+                validateUpdateIsPossible();
+            }
+        }
+
+        @Override
+        public void commitTypes(Map<String, IDataType> typesAdded) {
+            TypeSystem.this.commitTypes(typesAdded);
+        }
+
+        public Map<String, IDataType> getTypesAdded() {
+            return new HashMap<>(transientTypes);
+        }
     }
 
     public class IdType {