Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CHE-4372; add unique constraing for factory name + user #4521

Merged
merged 8 commits into from
Mar 23, 2017
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.stream.Collectors;

import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Strings.isNullOrEmpty;
import static java.util.Objects.requireNonNull;
import static org.eclipse.che.api.factory.shared.Constants.HTML_SNIPPET_TYPE;
import static org.eclipse.che.api.factory.shared.Constants.IFRAME_SNIPPET_TYPE;
Expand All @@ -42,8 +43,12 @@
@Singleton
public class FactoryManager {

private final FactoryDao factoryDao;

@Inject
private FactoryDao factoryDao;
public FactoryManager(FactoryDao factoryDao) {
this.factoryDao = factoryDao;
}

/**
* Stores {@link Factory} instance.
Expand Down Expand Up @@ -82,6 +87,9 @@ public Factory saveFactory(Factory factory, Set<FactoryImage> images) throws Con
requireNonNull(factory);
final FactoryImpl newFactory = new FactoryImpl(factory, images);
newFactory.setId(NameGenerator.generate("factory", 16));
if (isNullOrEmpty(newFactory.getName())) {
newFactory.setName(NameGenerator.generate("f", 9));
}
return factoryDao.create(newFactory);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/*******************************************************************************
* Copyright (c) 2012-2017 Codenvy, S.A.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
*
* Contributors:
* Codenvy, S.A. - initial API and implementation
*******************************************************************************/
package org.eclipse.che.api.factory.server;

import org.eclipse.che.api.factory.server.model.impl.FactoryImpl;
import org.eclipse.che.api.factory.server.spi.FactoryDao;
import org.mockito.ArgumentCaptor;
import org.mockito.Captor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.testng.MockitoTestNGListener;
import org.testng.annotations.Listeners;
import org.testng.annotations.Test;

import static com.google.common.base.Strings.isNullOrEmpty;
import static org.mockito.Mockito.verify;
import static org.testng.Assert.assertFalse;

/**
* @author Max Shaposhnik (mshaposhnik@codenvy.com) on 3/20/17.
*/
@Listeners(value = {MockitoTestNGListener.class})
public class FactoryManagerTest {

@Mock
private FactoryDao factoryDao;

@InjectMocks
private FactoryManager factoryManager;

@Captor
private ArgumentCaptor<FactoryImpl> factoryCaptor;

@Test
public void shouldGenerateNameOnFactoryCreation() throws Exception {
final FactoryImpl factory = FactoryImpl.builder().generateId().build();
factoryManager.saveFactory(factory);
verify(factoryDao).create(factoryCaptor.capture());
assertFalse(isNullOrEmpty(factoryCaptor.getValue().getName()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -131,15 +131,14 @@ public void shouldThrowConflictExceptionWhenCreatingFactoryWithExistingId() thro
factoryDao.create(factory);
}

// TODO fix after issue: https://github.com/eclipse/che/issues/2110
// @Test(expectedExceptions = ConflictException.class)
// public void shouldThrowConflictExceptionWhenCreatingFactoryWithExistingNameAndUserId() throws Exception {
// final FactoryImpl factory = createFactory(10, users[0].getId());
// final FactoryImpl existing = factories[0];
// factory.getCreator().setUserId(existing.getCreator().getUserId());
// factory.setName(existing.getName());
// factoryDao.create(factory);
// }
@Test(expectedExceptions = ConflictException.class)
public void shouldThrowConflictExceptionWhenCreatingFactoryWithExistingNameAndUserId() throws Exception {
final FactoryImpl factory = createFactory(10, users[0].getId());
final FactoryImpl existing = factories[0];
factory.getCreator().setUserId(existing.getCreator().getUserId());
factory.setName(existing.getName());
factoryDao.create(factory);
}

@Test
public void shouldUpdateFactory() throws Exception {
Expand All @@ -160,14 +159,13 @@ public void shouldUpdateFactory() throws Exception {
assertEquals(factoryDao.getById(update.getId()), update);
}

// TODO fix after issue: https://github.com/eclipse/che/issues/2110
// @Test(expectedExceptions = ConflictException.class)
// public void shouldThrowConflictExceptionWhenUpdateFactoryWithExistingNameAndUserId() throws Exception {
// final FactoryImpl update = factories[0];
// update.setName(factories[1].getName());
// update.getCreator().setUserId(factories[1].getCreator().getUserId());
// factoryDao.update(update);
// }
@Test(expectedExceptions = ConflictException.class)
public void shouldThrowConflictExceptionWhenUpdateFactoryWithExistingNameAndUserId() throws Exception {
final FactoryImpl update = factories[0];
update.setName(factories[1].getName());
update.getCreator().setUserId(factories[1].getCreator().getUserId());
factoryDao.update(update);
}

@Test(expectedExceptions = NullPointerException.class)
public void shouldThrowNpeWhenFactoryUpdateIsNull() throws Exception {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
--
-- [2012] - [2017] Codenvy, S.A.
-- All Rights Reserved.
--
-- NOTICE: All information contained herein is, and remains
-- the property of Codenvy S.A. and its suppliers,
-- if any. The intellectual and technical concepts contained
-- herein are proprietary to Codenvy S.A.
-- and its suppliers and may be covered by U.S. and Foreign Patents,
-- patents in process, and are protected by trade secret or copyright law.
-- Dissemination of this information or reproduction of this material
-- is strictly forbidden unless prior written permission is obtained
-- from Codenvy S.A..
--

CREATE UNIQUE INDEX index_che_factory_name_user_id ON che_factory (user_id, name);
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
--
-- [2012] - [2017] Codenvy, S.A.
-- All Rights Reserved.
--
-- NOTICE: All information contained herein is, and remains
-- the property of Codenvy S.A. and its suppliers,
-- if any. The intellectual and technical concepts contained
-- herein are proprietary to Codenvy S.A.
-- and its suppliers and may be covered by U.S. and Foreign Patents,
-- patents in process, and are protected by trade secret or copyright law.
-- Dissemination of this information or reproduction of this material
-- is strictly forbidden unless prior written permission is obtained
-- from Codenvy S.A..
--


-- Generate names where they are not set.
UPDATE che_factory
SET name = concat('f', right(id, 9)) WHERE name IS NULL OR name = '';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Script name does not match the content


-- Make names unique for the same user, e.g if there is more than one factory with same name and user,
-- leave first one and rename others.
WITH dupes AS
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this migration on codenvy tables before we move factory to che?
Because it's weird to see such a postgresql specific migration in che, while che doesn't have factories yet.
Moreover it allows to define unique constraint next to che_factory declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes I can move naming uniques into 1.1 in codenvy, then 1.2 will do the migration, and costraint will be just in che_factory table definition.

( SELECT id, user_id, name
FROM che_factory
WHERE (name,user_id)
IN (SELECT name, user_id
FROM che_factory
GROUP BY name, user_id
HAVING count(*) > 1)
),
uniques AS
( WITH q as
( SELECT *, row_number()
OVER (PARTITION BY name,user_id ORDER BY name,user_id)
AS rn FROM che_factory
)
SELECT id FROM q WHERE rn = 1
)
UPDATE che_factory
SET name = concat(che_factory.name, '-', right(dupes.id, 9))
FROM dupes, uniques
WHERE dupes.id = che_factory.id AND NOT EXISTS (SELECT id FROM uniques WHERE che_factory.id = uniques.id);

-- Create composite unique index.
CREATE UNIQUE INDEX index_che_factory_name_user_id ON che_factory (user_id, name);