Synchronize project attributes before storing workspace (#2726)

* Synchronize attribute before storing workspace

* fixup! Synchronize attribute before storing workspace

* fixup! fixup! Synchronize attribute before storing workspace

wsmaster/che-core-api-factory/src/main/java/org/eclipse/che/api/factory/server/jpa/JpaFactoryDao.java

@@ -22,6 +22,7 @@
 import org.eclipse.che.api.factory.server.model.impl.FactoryImpl;
 import org.eclipse.che.api.factory.server.spi.FactoryDao;
 import org.eclipse.che.api.user.server.event.BeforeUserRemovedEvent;
+import org.eclipse.che.api.workspace.server.model.impl.ProjectConfigImpl;
 import org.eclipse.che.commons.lang.NameGenerator;
 import org.eclipse.che.commons.lang.Pair;
 import org.slf4j.Logger;
@@ -141,6 +142,9 @@ public List<FactoryImpl> getByAttribute(int maxItems,
     @Transactional
     protected void doCreate(FactoryImpl factory) {
         final EntityManager manager = managerProvider.get();
+        if (factory.getWorkspace() != null) {
+            factory.getWorkspace().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         manager.persist(factory);
     }
 
@@ -150,6 +154,9 @@ protected FactoryImpl doUpdate(FactoryImpl update) throws NotFoundException {
         if (manager.find(FactoryImpl.class, update.getId()) == null) {
             throw new NotFoundException(format("Could not update factory with id %s because it doesn't exist", update.getId()));
         }
+        if (update.getWorkspace() != null) {
+            update.getWorkspace().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         return manager.merge(update);
     }
 

wsmaster/che-core-api-factory/src/test/java/org/eclipse/che/api/factory/server/spi/tck/FactoryDaoTest.java

@@ -257,17 +257,18 @@ private static FactoryImpl createFactory(int index, String userId) {
         final List<ActionImpl> a3 = new ArrayList<>(singletonList(new ActionImpl("id" + index, ImmutableMap.of("key3", "value3"))));
         final OnAppClosedImpl onAppClosed = new OnAppClosedImpl(a3);
         final IdeImpl ide = new IdeImpl(onAppLoaded, onProjectsLoaded, onAppClosed);
-        return FactoryImpl.builder()
-                          .generateId()
-                          .setVersion("4_0")
-                          .setName("factoryName" + index)
-                          .setWorkspace(createWorkspaceConfig(index))
-                          .setButton(factoryButton)
-                          .setCreator(creator)
-                          .setPolicies(policies)
-                          .setImages(images)
-                          .setIde(ide)
-                          .build();
+        final FactoryImpl factory = FactoryImpl.builder()
+                                               .generateId()
+                                               .setVersion("4_0")
+                                               .setName("factoryName" + index)
+                                               .setButton(factoryButton)
+                                               .setCreator(creator)
+                                               .setPolicies(policies)
+                                               .setImages(images)
+                                               .setIde(ide)
+                                               .build();
+        factory.setWorkspace(createWorkspaceConfig(index));
+        return factory;
     }
 
     public static WorkspaceConfigImpl createWorkspaceConfig(int index) {
@@ -363,6 +364,9 @@ public static WorkspaceConfigImpl createWorkspaceConfig(int index) {
         wCfg.setCommands(commands);
         wCfg.setProjects(projects);
         wCfg.setEnvironments(environments);
+
+        wCfg.getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+
         return wCfg;
     }
 }

wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/jpa/JpaStackDao.java

@@ -18,6 +18,7 @@
 import org.eclipse.che.api.core.jdbc.jpa.DuplicateKeyException;
 import org.eclipse.che.api.core.notification.EventService;
 import org.eclipse.che.api.workspace.server.event.StackPersistedEvent;
+import org.eclipse.che.api.workspace.server.model.impl.ProjectConfigImpl;
 import org.eclipse.che.api.workspace.server.model.impl.stack.StackImpl;
 import org.eclipse.che.api.workspace.server.spi.StackDao;
 import org.eclipse.che.commons.annotation.Nullable;
@@ -123,6 +124,9 @@ public List<StackImpl> searchStacks(@Nullable String user,
 
     @Transactional
     protected void doCreate(StackImpl stack) {
+        if (stack.getWorkspaceConfig() != null) {
+            stack.getWorkspaceConfig().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         managerProvider.get().persist(stack);
     }
 
@@ -141,6 +145,9 @@ protected StackImpl doUpdate(StackImpl update) throws NotFoundException {
         if (manager.find(StackImpl.class, update.getId()) == null) {
             throw new NotFoundException(format("Workspace with id '%s' doesn't exist", update.getId()));
         }
+        if (update.getWorkspaceConfig() != null) {
+            update.getWorkspaceConfig().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         return manager.merge(update);
     }
 }

wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/jpa/JpaWorkspaceDao.java

@@ -22,6 +22,7 @@
 import org.eclipse.che.api.machine.server.model.impl.SnapshotImpl;
 import org.eclipse.che.api.machine.server.spi.SnapshotDao;
 import org.eclipse.che.api.workspace.server.event.BeforeWorkspaceRemovedEvent;
+import org.eclipse.che.api.workspace.server.model.impl.ProjectConfigImpl;
 import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl;
 import org.eclipse.che.api.workspace.server.spi.WorkspaceDao;
 import org.slf4j.Logger;
@@ -154,6 +155,9 @@ public List<WorkspaceImpl> getWorkspaces(String userId) throws ServerException {
 
     @Transactional
     protected void doCreate(WorkspaceImpl workspace) {
+        if (workspace.getConfig() != null) {
+            workspace.getConfig().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         manager.get().persist(workspace);
     }
 
@@ -170,6 +174,9 @@ protected WorkspaceImpl doUpdate(WorkspaceImpl update) throws NotFoundException
         if (manager.get().find(WorkspaceImpl.class, update.getId()) == null) {
             throw new NotFoundException(format("Workspace with id '%s' doesn't exist", update.getId()));
         }
+        if (update.getConfig() != null) {
+            update.getConfig().getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+        }
         return manager.get().merge(update);
     }
 

wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/model/impl/ProjectConfigImpl.java

@@ -11,8 +11,8 @@
 package org.eclipse.che.api.workspace.server.model.impl;
 
 
-import org.eclipse.che.api.core.model.project.SourceStorage;
 import org.eclipse.che.api.core.model.project.ProjectConfig;
+import org.eclipse.che.api.core.model.project.SourceStorage;
 
 import javax.persistence.Basic;
 import javax.persistence.CascadeType;
@@ -27,7 +27,9 @@
 import javax.persistence.OneToMany;
 import javax.persistence.OneToOne;
 import javax.persistence.PostLoad;
+import javax.persistence.PrePersist;
 import javax.persistence.PreUpdate;
+import javax.persistence.Transient;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -72,10 +74,9 @@ public class ProjectConfigImpl implements ProjectConfig {
     @MapKey(name = "name")
     private Map<String, Attribute> dbAttributes;
 
-    // TODO consider using List<Attribute> or Map<String, Attribute> on model level instead
     // Mapping delegated to 'dbAttributes' field
     // as it is impossible to map nested list directly
-    @Column(insertable = false, updatable = false)
+    @Transient
     private Map<String, List<String>> attributes;
 
     public ProjectConfigImpl() {}
@@ -207,15 +208,32 @@ public String toString() {
                '}';
     }
 
-    @PreUpdate
-    public void syncDbAttributes() {
-        dbAttributes = getAttributes().entrySet()
-                                      .stream()
-                                      .collect(toMap(Map.Entry::getKey, e -> new Attribute(e.getKey(), e.getValue())));
+    /**
+     * Synchronizes instance attribute with db attributes,
+     * should be called by internal components in needed places,
+     * this can't be done neither by {@link PrePersist} nor by {@link PreUpdate}
+     * as when the entity is merged the transient attribute won't be passed
+     * to event handlers.
+     */
+    public void prePersistAttributes() {
+        if (dbAttributes == null) {
+            dbAttributes = new HashMap<>();
+        }
+        final Map<String, Attribute> dbAttrsCopy = new HashMap<>(dbAttributes);
+        dbAttributes.clear();
+        for (Map.Entry<String, List<String>> entry : getAttributes().entrySet()) {
+            Attribute attribute = dbAttrsCopy.get(entry.getKey());
+            if (attribute == null) {
+                attribute = new Attribute(entry.getKey(), entry.getValue());
+            } else if (!Objects.equals(attribute.values, entry.getValue())) {
+                attribute.values = entry.getValue();
+            }
+            dbAttributes.put(entry.getKey(), attribute);
+        }
     }
 
     @PostLoad
-    private void initEntityAttributes() {
+    private void postLoadAttributes() {
         attributes = dbAttributes.values()
                                  .stream()
                                  .collect(toMap(attr -> attr.name, attr -> attr.values));

wsmaster/che-core-api-workspace/src/main/java/org/eclipse/che/api/workspace/server/model/impl/WorkspaceConfigImpl.java

@@ -214,12 +214,6 @@ public String toString() {
                '}';
     }
 
-    @PreUpdate
-    @PrePersist
-    public void syncProjects() {
-        getProjects().forEach(ProjectConfigImpl::syncDbAttributes);
-    }
-
     /**
      * Helps to build complex {@link WorkspaceConfigImpl users workspace instance}.
      *

wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/jpa/JpaWorkspaceDaoTest.java

@@ -11,10 +11,10 @@
 package org.eclipse.che.api.workspace.server.jpa;
 
 import com.google.inject.Guice;
+import com.google.inject.Injector;
 
 import org.eclipse.che.account.spi.AccountImpl;
 import org.eclipse.che.api.core.jdbc.jpa.DuplicateKeyException;
-import org.eclipse.che.api.workspace.server.model.impl.ProjectConfigImpl;
 import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl;
 import org.eclipse.che.api.workspace.server.model.impl.WorkspaceImpl;
 import org.testng.annotations.AfterMethod;
@@ -23,10 +23,10 @@
 
 import javax.persistence.EntityManager;
 
-import java.util.Collections;
-import java.util.HashMap;
+import java.util.ArrayList;
+import java.util.List;
 
-import static java.util.Arrays.asList;
+import static java.util.Collections.singletonList;
 import static org.eclipse.che.api.workspace.server.spi.tck.WorkspaceDaoTest.createWorkspace;
 import static org.testng.Assert.assertEquals;
 
@@ -37,18 +37,25 @@
  */
 public class JpaWorkspaceDaoTest {
 
-    private EntityManager manager;
+    private EntityManager   manager;
+    private JpaWorkspaceDao workspaceDao;
 
     @BeforeMethod
     private void setUpManager() {
-        manager = Guice.createInjector(new WorkspaceTckModule()).getInstance(EntityManager.class);
+        final Injector injector = Guice.createInjector(new WorkspaceTckModule());
+        manager = injector.getInstance(EntityManager.class);
+        workspaceDao = injector.getInstance(JpaWorkspaceDao.class);
     }
 
     @AfterMethod
     private void cleanup() {
         manager.getTransaction().begin();
-        manager.createQuery("DELETE FROM Workspace workspaces");
-        manager.createQuery("DELETE FROM Account accounts");
+        final List<Object> entities = new ArrayList<>();
+        entities.addAll(manager.createQuery("SELECT w FROM Workspace w").getResultList());
+        entities.addAll(manager.createQuery("SELECT a FROM Account a").getResultList());
+        for (Object entity : entities) {
+            manager.remove(entity);
+        }
         manager.getTransaction().commit();
         manager.getEntityManagerFactory().close();
     }
@@ -107,6 +114,37 @@ public void shouldSynchronizeWorkspaceNameWithConfigNameWhenConfigIsUpdated() th
         manager.getTransaction().commit();
     }
 
+    @Test
+    public void shouldSyncDbAttributesWhileUpdatingWorkspace() throws Exception {
+        final AccountImpl account = new AccountImpl("accountId", "namespace", "test");
+        final WorkspaceImpl workspace = createWorkspace("id", account, "name");
+
+        // persist the workspace
+        manager.getTransaction().begin();
+        manager.persist(account);
+        manager.persist(workspace);
+        manager.getTransaction().commit();
+        manager.clear();
+
+        // put a new attribute
+        workspace.getConfig()
+                 .getProjects()
+                 .get(0)
+                 .getAttributes()
+                 .put("new-attr", singletonList("value"));
+        workspaceDao.update(workspace);
+
+        manager.clear();
+
+        // check it's okay
+        assertEquals(workspaceDao.get(workspace.getId())
+                                 .getConfig()
+                                 .getProjects()
+                                 .get(0)
+                                 .getAttributes()
+                                 .size(), 3);
+    }
+
     private long asLong(String query) {
         return manager.createQuery(query, Long.class).getSingleResult();
     }

wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/tck/StackDaoTest.java

@@ -18,6 +18,7 @@
 import org.eclipse.che.api.core.notification.EventService;
 import org.eclipse.che.api.machine.server.spi.SnapshotDao;
 import org.eclipse.che.api.workspace.server.event.StackPersistedEvent;
+import org.eclipse.che.api.workspace.server.model.impl.WorkspaceConfigImpl;
 import org.eclipse.che.api.workspace.server.model.impl.stack.StackComponentImpl;
 import org.eclipse.che.api.workspace.server.model.impl.stack.StackImpl;
 import org.eclipse.che.api.workspace.server.model.impl.stack.StackSourceImpl;
@@ -242,20 +243,22 @@ private void updateAll() throws ConflictException, NotFoundException, ServerExce
     }
 
     private static StackImpl createStack(String id, String name) {
-        return StackImpl.builder()
-                        .setId(id)
-                        .setName(name)
-                        .setCreator("user123")
-                        .setDescription(id + "-description")
-                        .setScope(id + "-scope")
-                        .setWorkspaceConfig(createWorkspaceConfig("test"))
-                        .setTags(asList(id + "-tag1", id + "-tag2"))
-                        .setComponents(asList(new StackComponentImpl(id + "-component1", id + "-component1-version"),
-                                              new StackComponentImpl(id + "-component2", id + "-component2-version")))
-                        .setSource(new StackSourceImpl(id + "-type", id + "-origin"))
-                        .setStackIcon(new StackIcon(id + "-icon",
-                                                    id + "-media-type",
-                                                    "0x1234567890abcdef".getBytes()))
-                        .build();
+        final StackImpl stack = StackImpl.builder()
+                                     .setId(id)
+                                     .setName(name)
+                                     .setCreator("user123")
+                                     .setDescription(id + "-description")
+                                     .setScope(id + "-scope")
+                                     .setTags(asList(id + "-tag1", id + "-tag2"))
+                                     .setComponents(asList(new StackComponentImpl(id + "-component1", id + "-component1-version"),
+                                                           new StackComponentImpl(id + "-component2", id + "-component2-version")))
+                                     .setSource(new StackSourceImpl(id + "-type", id + "-origin"))
+                                     .setStackIcon(new StackIcon(id + "-icon",
+                                                                 id + "-media-type",
+                                                                 "0x1234567890abcdef".getBytes()))
+                                     .build();
+        final WorkspaceConfigImpl config = createWorkspaceConfig("test");
+        stack.setWorkspaceConfig(config);
+        return stack;
     }
 }

wsmaster/che-core-api-workspace/src/test/java/org/eclipse/che/api/workspace/server/spi/tck/WorkspaceDaoTest.java

@@ -455,8 +455,8 @@ public static WorkspaceConfigImpl createWorkspaceConfig(String name) {
         recipe2.setContentType("text/x-dockerfile");
         recipe2.setContent("content");
         final EnvironmentImpl env2 = new EnvironmentImpl();
-        env2.setMachines(new HashMap<>(ImmutableMap.of("machine1", exMachine1,
-                                                       "machine3", exMachine3)));
+        env2.setMachines(new HashMap<>(ImmutableMap.of("machine1", new ExtendedMachineImpl(exMachine1),
+                                                       "machine3", new ExtendedMachineImpl(exMachine3))));
         env2.setRecipe(recipe2);
 
         final Map<String, EnvironmentImpl> environments = ImmutableMap.of("env1", env1, "env2", env2);
@@ -469,6 +469,9 @@ public static WorkspaceConfigImpl createWorkspaceConfig(String name) {
         wCfg.setCommands(commands);
         wCfg.setProjects(projects);
         wCfg.setEnvironments(new HashMap<>(environments));
+
+        wCfg.getProjects().forEach(ProjectConfigImpl::prePersistAttributes);
+
         return wCfg;
     }
 
@@ -478,9 +481,8 @@ public static WorkspaceImpl createWorkspace(String id, AccountImpl account, Stri
         final WorkspaceImpl workspace = new WorkspaceImpl();
         workspace.setId(id);
         workspace.setAccount(account);
-        final WorkspaceConfigImpl cfg = new WorkspaceConfigImpl();
-        cfg.setName(name);
-        workspace.setConfig(cfg);
+        wCfg.setName(name);
+        workspace.setConfig(wCfg);
         workspace.setAttributes(new HashMap<>(ImmutableMap.of("attr1", "value1",
                                                               "attr2", "value2",
                                                               "attr3", "value3")));