Skip to content

Commit

Permalink
Moved the etag generation code related to EnvConfig(s) to EntityHashi…
Browse files Browse the repository at this point in the history 
…ngService

Added the fix for issue gocd#6682
  • Loading branch information
@kritika-singh3
kritika-singh3 committed Sep 9, 2019
1 parent be36a46 commit 801b268
Show file tree
Hide file tree
Showing 7 changed files with 156 additions and 41 deletions.
2 changes: 1 addition & 1 deletion api/api-agents-v6/src/main/java/com/thoughtworks/go/apiv6/agents/AgentsControllerV6.java
View file
Expand Up @@ -217,7 +217,7 @@ public AgentInstance doFetchEntityFromConfig(String uuid) {

@Override
public AgentInstance buildEntityFromRequestBody(Request req) {
return null;
throw new UnsupportedOperationException();
}

@Override
Expand Down
35 changes: 14 additions & 21 deletions ...nts-v3/src/main/java/com/thoughtworks/go/apiv3/environments/EnvironmentsControllerV3.java
View file
Expand Up @@ -31,34 +31,31 @@
import com.thoughtworks.go.config.exceptions.EntityType;
import com.thoughtworks.go.config.exceptions.HttpException;
import com.thoughtworks.go.config.exceptions.RecordNotFoundException;
import com.thoughtworks.go.config.merge.MergeEnvironmentConfig;
import com.thoughtworks.go.domain.ConfigElementForEdit;
import com.thoughtworks.go.server.service.EntityHashingService;
import com.thoughtworks.go.server.service.EnvironmentConfigService;
import com.thoughtworks.go.server.service.result.HttpLocalizedOperationResult;
import com.thoughtworks.go.spark.Routes;
import com.thoughtworks.go.spark.spring.SparkSpringController;
import org.apache.commons.codec.digest.DigestUtils;
import org.apache.commons.lang3.StringUtils;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Component;
import spark.Request;
import spark.Response;

import java.io.IOException;
import java.util.*;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.function.Consumer;
import java.util.stream.Collectors;

import static com.thoughtworks.go.api.representers.EnvironmentVariableRepresenter.toJSONArray;
import static com.thoughtworks.go.api.util.HaltApiResponses.*;
import static com.thoughtworks.go.apiv3.environments.representers.EnvironmentsRepresenter.toJSON;
import static java.lang.String.format;
import static java.lang.String.valueOf;
import static java.util.Comparator.comparing;
import static java.util.Objects.hash;
import static java.util.stream.Collectors.toList;
import static org.apache.commons.codec.digest.DigestUtils.md5Hex;
import static java.util.stream.Collectors.toCollection;
import static spark.Spark.*;

@Component
Expand Down Expand Up @@ -103,7 +100,7 @@ public void setupRoutes() {

public String index(Request request, Response response) throws IOException {
Set<EnvironmentConfig> envConfigSet = environmentConfigService.getEnvironments();
List<EnvironmentConfig> envViewModelList = filterUnknownAndSortEnvConfigs(envConfigSet);
EnvironmentsConfig envViewModelList = filterUnknownAndSortEnvConfigs(envConfigSet);
setEtagHeader(response, calculateEtag(envViewModelList));
return writerForTopLevelObject(request, response, outputWriter -> toJSON(outputWriter, envViewModelList));
}
Expand All @@ -113,6 +110,10 @@ public String show(Request request, Response response) throws IOException {

EnvironmentConfig environmentConfig = fetchEntityFromConfig(environmentName);

String etag = etagFor(environmentConfig);
if (fresh(request, etag)) {
return notModified(response);
}
setEtagHeader(environmentConfig, response);

return writerForTopLevelObject(request, response,
Expand Down Expand Up @@ -203,9 +204,6 @@ public String remove(Request request, Response response) {

@Override
public String etagFor(EnvironmentConfig entityFromServer) {
if (entityFromServer instanceof MergeEnvironmentConfig || entityFromServer instanceof UnknownEnvironmentConfig) {
return md5Hex(valueOf(hash(entityFromServer)));
}
return entityHashingService.md5ForEntity(entityFromServer);
}

Expand Down Expand Up @@ -239,11 +237,11 @@ public Consumer<OutputWriter> jsonWriter(EnvironmentConfig environmentConfig) {
return writer -> EnvironmentRepresenter.toJSON(writer, environmentConfig);
}

List<EnvironmentConfig> filterUnknownAndSortEnvConfigs(Set<EnvironmentConfig> envConfigSet) {
EnvironmentsConfig filterUnknownAndSortEnvConfigs(Set<EnvironmentConfig> envConfigSet) {
return envConfigSet.stream()
.filter(envConfig -> !envConfig.isUnknown())
.sorted(comparing(EnvironmentConfig::name))
.collect(toList());
.collect(toCollection(EnvironmentsConfig::new));
}

private void haltIfEntityWithSameNameExists(EnvironmentConfig environmentConfig) {
Expand All @@ -257,12 +255,7 @@ private void haltIfEntityWithSameNameExists(EnvironmentConfig environmentConfig)
throw haltBecauseEntityAlreadyExists(jsonWriter(environmentConfig), "environment", environmentConfig.name().toString());
}

private String calculateEtag(Collection<EnvironmentConfig> environmentConfigs) {
final String environmentConfigSegment = environmentConfigs
.stream()
.map(this::etagFor)
.collect(Collectors.joining(SEP_CHAR));

return DigestUtils.sha256Hex(environmentConfigSegment);
private String calculateEtag(EnvironmentsConfig envConfigs) {
return entityHashingService.md5ForEntity(envConfigs);
}
}
39 changes: 39 additions & 0 deletions ...rc/test/groovy/com/thoughtworks/go/apiv3/environments/EnvironmentsControllerV3Test.groovy
View file
Expand Up @@ -317,6 +317,44 @@ class EnvironmentsControllerV3Test implements SecurityServiceTrait, ControllerTr
.isNotFound()
.hasJsonMessage(controller.entityType.notFoundMessage("env1"))
}

@Test
void 'should render not modified when ETag matches'() {
def env1 = new BasicEnvironmentConfig(new CaseInsensitiveString("env1"))
env1.addAgent("agent1")
env1.addAgent("agent2")
env1.addEnvironmentVariable("JAVA_HOME", "/bin/java")
env1.addPipeline(new CaseInsensitiveString("Pipeline1"))
env1.addPipeline(new CaseInsensitiveString("Pipeline2"))

when(entityHashingService.md5ForEntity(env1)).thenReturn("md5-hash")
when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(env1)

getWithApiHeader(controller.controllerPath("env1"), ['if-none-match': 'md5-hash'])

assertThatResponse()
.isNotModified()
}

@Test
void 'should return 200 when ETag does not match'() {
def env1 = new BasicEnvironmentConfig(new CaseInsensitiveString("env1"))
env1.addAgent("agent1")
env1.addAgent("agent2")
env1.addEnvironmentVariable("JAVA_HOME", "/bin/java")
env1.addPipeline(new CaseInsensitiveString("Pipeline1"))
env1.addPipeline(new CaseInsensitiveString("Pipeline2"))

when(entityHashingService.md5ForEntity(env1)).thenReturn("md5-hash")
when(environmentConfigService.getEnvironmentConfig(eq("env1"))).thenReturn(env1)

getWithApiHeader(controller.controllerPath("env1"), ['if-none-match': 'md5-old-hash'])

assertThatResponse()
.isOk()
.hasEtag('"md5-hash"')
.hasBodyWithJsonObject(env1, EnvironmentRepresenter)
}
}
}

Expand Down Expand Up @@ -671,6 +709,7 @@ class EnvironmentsControllerV3Test implements SecurityServiceTrait, ControllerTr
patchWithApiHeader(controller.controllerPath("env1"), patchRequestJson)

assertThatResponse()
.isUnprocessableEntity()
.hasJsonBody(expectedJson)
}
}
Expand Down
44 changes: 32 additions & 12 deletions server/src/main/java/com/thoughtworks/go/server/service/EntityHashingService.java
View file
Expand Up @@ -19,6 +19,7 @@
import com.thoughtworks.go.config.*;
import com.thoughtworks.go.config.elastic.ClusterProfile;
import com.thoughtworks.go.config.elastic.ElasticProfile;
import com.thoughtworks.go.config.merge.MergeEnvironmentConfig;
import com.thoughtworks.go.config.merge.MergePipelineConfigs;
import com.thoughtworks.go.config.registry.ConfigElementImplementationRegistry;
import com.thoughtworks.go.config.remote.ConfigRepoConfig;
Expand Down Expand Up @@ -49,9 +50,15 @@
import java.util.List;
import java.util.Objects;
import java.util.function.Supplier;
import java.util.stream.Collectors;

import static java.lang.String.valueOf;
import static java.util.Objects.hash;
import static org.apache.commons.codec.digest.DigestUtils.md5Hex;

@Component
public class EntityHashingService implements ConfigChangedListener, Initializer {
private static final String SEP_CHAR = "/";
private GoConfigService goConfigService;
private GoCache goCache;
private static final String ETAG_CACHE_KEY = "GO_ETAG_CACHE".intern();
Expand Down Expand Up @@ -102,10 +109,23 @@ public String md5ForEntity(PipelineTemplateConfig config) {
}

public String md5ForEntity(EnvironmentConfig config) {
if (config instanceof MergeEnvironmentConfig || config instanceof UnknownEnvironmentConfig) {
return md5Hex(valueOf(hash(config)));
}

String cacheKey = cacheKey(config, config.name());
return getDomainEntityMd5FromCache(config, cacheKey);
}

public String md5ForEntity(EnvironmentsConfig envConfigs) {
final String environmentConfigSegment = envConfigs
.stream()
.map(this::md5ForEntity)
.collect(Collectors.joining(SEP_CHAR));

return CachedDigestUtils.sha256Hex(environmentConfigSegment);
}

public String md5ForEntity(PackageRepository config) {
String cacheKey = cacheKey(config, config.getId());
return getDomainEntityMd5FromCache(config, cacheKey);
Expand All @@ -116,7 +136,7 @@ public String md5ForEntity(PackageRepositories packageRepositories) {
for (PackageRepository packageRepository : packageRepositories) {
md5s.add(md5ForEntity(packageRepository));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(SCM config) {
Expand Down Expand Up @@ -160,7 +180,7 @@ public String md5ForEntity(CommandSnippets commandSnippets) {
md5s.add(md5ForEntity(commandSnippet));
}

return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(SecurityAuthConfig config) {
Expand All @@ -173,7 +193,7 @@ public String md5ForEntity(SecurityAuthConfigs authConfigs) {
for (SecurityAuthConfig authConfig : authConfigs) {
md5s.add(md5ForEntity(authConfig));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(Role config) {
Expand All @@ -196,7 +216,7 @@ public String md5ForEntity(Packages config) {
for (PackageDefinition packageDefinition : config) {
md5s.add(md5ForEntity(packageDefinition));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(PluginSettings pluginSettings) {
Expand Down Expand Up @@ -262,7 +282,7 @@ public String md5ForEntity(RolesConfig roles) {
for (Role role : roles) {
md5s.add(md5ForEntity(role));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(UsageStatisticsReporting usageStatisticsReporting) {
Expand All @@ -280,7 +300,7 @@ public String md5ForEntity(PipelineGroups pipelineGroups) {
for (PipelineConfigs pipelineConfigs : pipelineGroups) {
md5s.add(md5ForEntity(pipelineConfigs));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(PipelineConfigs pipelineConfigs) {
Expand All @@ -295,26 +315,26 @@ public String md5ForEntity(CombinedPluginInfo pluginInfo) {

public String md5ForEntity(Collection<CombinedPluginInfo> pluginInfos) {
List<String> md5s = new ArrayList<>();
for(CombinedPluginInfo pluginInfo: pluginInfos) {
for (CombinedPluginInfo pluginInfo : pluginInfos) {
md5s.add(md5ForEntity(pluginInfo));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(SecretConfigs secretConfigs) {
List<String> md5s = new ArrayList<>();
for(SecretConfig secretConfig: secretConfigs) {
for (SecretConfig secretConfig : secretConfigs) {
md5s.add(md5ForEntity(secretConfig));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

public String md5ForEntity(SCMs scms) {
List<String> md5s = new ArrayList<>();
for(SCM scm : scms) {
for (SCM scm : scms) {
md5s.add(md5ForEntity(scm));
}
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, "/"));
return CachedDigestUtils.md5Hex(StringUtils.join(md5s, SEP_CHAR));
}

class PipelineConfigChangedListener extends EntityConfigChangedListener<PipelineConfig> {
Expand Down
15 changes: 13 additions & 2 deletions ...er/src/test-fast/java/com/thoughtworks/go/config/update/DeleteEnvironmentCommandTest.java
View file
Expand Up @@ -56,20 +56,31 @@ public void setup() throws Exception {
}

@Test
public void shouldDeleteTheSpecifiedEnvironment() throws Exception {
public void shouldDeleteTheSpecifiedEnvironment() {
DeleteEnvironmentCommand command = new DeleteEnvironmentCommand(goConfigService, environmentConfig, currentUser, actionFailed, result);
assertTrue(cruiseConfig.getEnvironments().hasEnvironmentNamed(environmentName));
command.update(cruiseConfig);
assertFalse(cruiseConfig.getEnvironments().hasEnvironmentNamed(environmentName));
}

@Test
public void shouldNotContinueIfTheUserDoesNotHavePermissionsToOperateOnEnvironments() throws Exception {
public void shouldNotContinueIfTheUserDoesNotHavePermissionsToOperateOnEnvironments() {
DeleteEnvironmentCommand command = new DeleteEnvironmentCommand(goConfigService, environmentConfig, currentUser, actionFailed, result);
when(goConfigService.isUserAdmin(currentUser)).thenReturn(false);
assertThat(command.canContinue(cruiseConfig), is(false));
assertFalse(result.isSuccessful());
assertThat(result.httpCode(), is(403));
assertThat(result.message(), is(EntityType.Environment.forbiddenToEdit(environmentConfig.name(), currentUser.getUsername())));
}

@Test
public void shouldBeAbleToDeleteEvenIfTheSpecifiedEnvContainsAgents() {
BasicEnvironmentConfig environmentConfig = new BasicEnvironmentConfig(environmentName);
environmentConfig.addAgent("uuid");
DeleteEnvironmentCommand command = new DeleteEnvironmentCommand(goConfigService, environmentConfig, currentUser, actionFailed, result);
assertTrue(cruiseConfig.getEnvironments().hasEnvironmentNamed(environmentName));
command.update(cruiseConfig);
assertFalse(cruiseConfig.getEnvironments().hasEnvironmentNamed(environmentName));

}
}
30 changes: 26 additions & 4 deletions server/src/test-fast/java/com/thoughtworks/go/server/service/EntityHashingServiceTest.java
View file
Expand Up @@ -15,10 +15,8 @@
*/
package com.thoughtworks.go.server.service;

import com.thoughtworks.go.config.ConfigCache;
import com.thoughtworks.go.config.EnvironmentConfig;
import com.thoughtworks.go.config.MagicalGoConfigXmlWriter;
import com.thoughtworks.go.config.PipelineConfig;
import com.thoughtworks.go.config.*;
import com.thoughtworks.go.config.merge.MergeEnvironmentConfig;
import com.thoughtworks.go.config.registry.ConfigElementImplementationRegistry;
import com.thoughtworks.go.helper.PipelineConfigMother;
import com.thoughtworks.go.server.cache.GoCache;
Expand Down Expand Up @@ -111,4 +109,28 @@ public void entityChecksumIsIdenticalForObjectsWithCaseInsensitiveName() throws
verify(goCache).get("GO_ETAG_CACHE", "com.thoughtworks.go.config.PipelineConfig.upper_case_name");
verifyNoMoreInteractions(goCache);
}

@Test
public void shouldNotAccessCacheIfTheEnvironmentConfigIsAnInstanceOfMergeOrUnknownEnvConfig() {
BasicEnvironmentConfig basicEnvConfig = new BasicEnvironmentConfig(new CaseInsensitiveString("env"));
MergeEnvironmentConfig mergeEnvConfig = new MergeEnvironmentConfig(basicEnvConfig);
UnknownEnvironmentConfig unknownEnvConfig = new UnknownEnvironmentConfig(new CaseInsensitiveString("unknown"));

entityHashingService.md5ForEntity(mergeEnvConfig);
entityHashingService.md5ForEntity(unknownEnvConfig);

verifyZeroInteractions(goCache);
}

@Test
public void shouldAccessCacheIfTheEnvironmentConfigIsAnInstanceOfBasicEnvConfig() {
BasicEnvironmentConfig basicEnvConfig = new BasicEnvironmentConfig(new CaseInsensitiveString("env"));
when(goCache.get("GO_ETAG_CACHE", "com.thoughtworks.go.config.BasicEnvironmentConfig.env")).thenReturn("foo");

String md5 = entityHashingService.md5ForEntity(basicEnvConfig);

assertThat(md5, is("foo"));
verify(goCache).get("GO_ETAG_CACHE", "com.thoughtworks.go.config.BasicEnvironmentConfig.env");
verifyNoMoreInteractions(goCache);
}
}

0 comments on commit 801b268

Please sign in to comment.