Skip to content

Commit

Permalink
Address comments from @ikhoon
Browse files Browse the repository at this point in the history
  • Loading branch information
@minwoox
minwoox committed Dec 17, 2024
1 parent ef403a9 commit 75da118
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 83 deletions.
4 changes: 1 addition & 3 deletions common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
import java.util.Iterator;
import java.util.Set;

import javax.annotation.Nullable;

import com.fasterxml.jackson.core.JsonFactory;
import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.core.JsonParseException;
Expand Down Expand Up @@ -238,7 +236,7 @@ public static String writeValueAsPrettyString(Object value) throws JsonProcessin
}
}

public static <T extends JsonNode> T valueToTree(@Nullable Object value) {
public static <T extends JsonNode> T valueToTree(Object value) {
return compactMapper.valueToTree(value);
}

Expand Down
124 changes: 45 additions & 79 deletions server/src/main/java/com/linecorp/centraldogma/server/metadata/MetadataService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,7 @@ public CompletableFuture<Revision> removeMember(Author author, String projectNam
final ProjectMetadataTransformer transformer =
new ProjectMetadataTransformer((headRevision, projectMetadata) -> {
projectMetadata.member(memberId); // Raises an exception if the member does not exist.
final Map<String, Member> newMembers =
projectMetadata.members().entrySet().stream()
.filter(entry -> !entry.getKey().equals(memberId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));

final Map<String, Member> newMembers = removeFromMap(projectMetadata.members(), memberId);
final ImmutableMap<String, RepositoryMetadata> newRepos =
removeMemberFromRepositories(projectMetadata, memberId);
return new ProjectMetadata(projectMetadata.name(),
Expand All @@ -284,10 +280,7 @@ private static ImmutableMap<String, RepositoryMetadata> removeMemberFromReposito
final Roles roles = repositoryMetadata.roles();
final Map<String, RepositoryRole> users = roles.users();
if (users.get(memberId) != null) {
final ImmutableMap<String, RepositoryRole> newUsers =
users.entrySet().stream()
.filter(e -> !e.getKey().equals(memberId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
final ImmutableMap<String, RepositoryRole> newUsers = removeFromMap(users, memberId);
final Roles newRoles = new Roles(roles.projectRoles(), newUsers, roles.tokens());
reposBuilder.put(entry.getKey(),
new RepositoryMetadata(repositoryMetadata.name(),
Expand Down Expand Up @@ -566,10 +559,7 @@ private static ImmutableMap<String, RepositoryMetadata> removeTokenFromRepositor
final RepositoryMetadata repositoryMetadata = entry.getValue();
final Roles roles = repositoryMetadata.roles();
if (roles.tokens().get(appId) != null) {
final Map<String, RepositoryRole> newTokens =
roles.tokens().entrySet().stream()
.filter(e -> !e.getKey().equals(appId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
final Map<String, RepositoryRole> newTokens = removeFromMap(roles.tokens(), appId);
final Roles newRoles = new Roles(roles.projectRoles(), roles.users(), newTokens);
builder.put(entry.getKey(), new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -630,11 +620,8 @@ public CompletableFuture<Revision> addUserRepositoryRole(Author author, String p
projectName + '/' + repoName + '\'');
}

final ImmutableMap<String, RepositoryRole> newUsers =
ImmutableMap.<String, RepositoryRole>builderWithExpectedSize(roles.users().size() + 1)
.putAll(roles.users())
.put(member.id(), role)
.build();
final Map<String, RepositoryRole> users = roles.users();
final ImmutableMap<String, RepositoryRole> newUsers = addToMap(users, member.id(), role);
final Roles newRoles = new Roles(roles.projectRoles(), newUsers, roles.tokens());
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -665,10 +652,7 @@ public CompletableFuture<Revision> removeUserRepositoryRole(Author author, Strin
throw new MemberNotFoundException(memberId, projectName, repoName);
}

final Map<String, RepositoryRole> newUsers =
roles.users().entrySet().stream()
.filter(entry -> !entry.getKey().equals(memberId))
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
final Map<String, RepositoryRole> newUsers = removeFromMap(roles.users(), memberId);
final Roles newRoles = new Roles(roles.projectRoles(), newUsers, roles.tokens());
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -710,8 +694,7 @@ public CompletableFuture<Revision> updateUserRepositoryRole(Author author, Strin
"' isn't changed.");
}

final Map<String, RepositoryRole> newUsers =
updateRepositoryRole(role, roles.users(), memberId);
final Map<String, RepositoryRole> newUsers = updateMap(roles.users(), memberId, role);
final Roles newRoles = new Roles(roles.projectRoles(), newUsers, roles.tokens());
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand All @@ -724,21 +707,6 @@ public CompletableFuture<Revision> updateUserRepositoryRole(Author author, Strin
return metadataRepo.push(projectName, Project.REPO_DOGMA, author, commitSummary, transformer);
}

private static Map<String, RepositoryRole> updateRepositoryRole(
RepositoryRole repositoryRole, Map<String, RepositoryRole> repositoryRoles,
String id) {
final ImmutableMap.Builder<String, RepositoryRole> builder =
ImmutableMap.builderWithExpectedSize(repositoryRoles.size());
for (Entry<String, RepositoryRole> entry : repositoryRoles.entrySet()) {
if (entry.getKey().equals(id)) {
builder.put(id, repositoryRole);
} else {
builder.put(entry);
}
}
return builder.build();
}

/**
* Adds the {@link RepositoryRole} for the {@link Token} of the specified {@code appId} to the specified
* {@code repoName} in the specified {@code projectName}.
Expand All @@ -765,10 +733,7 @@ public CompletableFuture<Revision> addTokenRepositoryRole(Author author, String
projectName + '/' + repoName + '\'');
}

final Map<String, RepositoryRole> newTokens =
ImmutableMap.<String, RepositoryRole>builderWithExpectedSize(roles.tokens().size() + 1)
.putAll(roles.tokens())
.put(appId, role).build();
final Map<String, RepositoryRole> newTokens = addToMap(roles.tokens(), appId, role);
final Roles newRoles = new Roles(roles.projectRoles(), roles.users(), newTokens);
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -800,10 +765,7 @@ public CompletableFuture<Revision> removeTokenRepositoryRole(Author author, Stri
projectName + '/' + repoName + '\'');
}

final Map<String, RepositoryRole> newTokens =
roles.tokens().entrySet().stream()
.filter(entry -> !entry.getKey().equals(appId))
.collect(toImmutableMap(Map.Entry::getKey, Map.Entry::getValue));
final Map<String, RepositoryRole> newTokens = removeFromMap(roles.tokens(), appId);
final Roles newRoles = new Roles(roles.projectRoles(), roles.users(), newTokens);
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -846,8 +808,7 @@ public CompletableFuture<Revision> updateTokenRepositoryRole(Author author, Stri
"' isn't changed.");
}

final Map<String, RepositoryRole> newTokens =
updateRepositoryRole(role, roles.tokens(), appId);
final Map<String, RepositoryRole> newTokens = updateMap(roles.tokens(), appId, role);
final Roles newRoles = new Roles(roles.projectRoles(), roles.users(), newTokens);
return new RepositoryMetadata(repositoryMetadata.name(),
newRoles,
Expand Down Expand Up @@ -941,7 +902,7 @@ private CompletableFuture<RepositoryRole> findRepositoryRole0(String projectName

@Nullable
private static RepositoryRole repositoryRole(Roles roles, @Nullable RepositoryRole repositoryRole,
@Nullable ProjectRole projectRole) {
ProjectRole projectRole) {
if (repositoryRole == RepositoryRole.ADMIN || projectRole == ProjectRole.OWNER) {
return RepositoryRole.ADMIN;
}
Expand Down Expand Up @@ -1067,24 +1028,11 @@ public CompletableFuture<Revision> destroyToken(Author author, String appId) {
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(),
token.isSystemAdmin(), token.creation(),
token.deactivation(), userAndTimestamp);
return new Tokens(newAppIds(tokens, appId, newToken), tokens.secrets());
return new Tokens(updateMap(tokens.appIds(), appId, newToken), tokens.secrets());
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
}

private static Map<String, Token> newAppIds(Tokens tokens, String appId, Token newToken) {
final ImmutableMap.Builder<String, Token> appIdsBuilder =
ImmutableMap.builderWithExpectedSize(tokens.appIds().size());
for (Entry<String, Token> entry : tokens.appIds().entrySet()) {
if (!entry.getKey().equals(appId)) {
appIdsBuilder.put(entry);
} else {
appIdsBuilder.put(appId, newToken);
}
}
return appIdsBuilder.build();
}

/**
* Purges the {@link Token} of the specified {@code appId} that was removed before.
*
Expand Down Expand Up @@ -1113,14 +1061,10 @@ public Revision purgeToken(Author author, String appId) {

final TokensTransformer transformer = new TokensTransformer((headRevision, tokens) -> {
final Token token = tokens.get(appId);
final Map<String, Token> newAppIds = tokens.appIds().entrySet().stream()
.filter(entry -> !entry.getKey().equals(appId))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
final Map<String, Token> newAppIds = removeFromMap(tokens.appIds(), appId);
final String secret = token.secret();
assert secret != null;
final Map<String, String> newSecrets =
tokens.secrets().entrySet().stream().filter(entry -> !entry.getKey().equals(secret))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
final Map<String, String> newSecrets = removeFromMap(tokens.secrets(), secret);
return new Tokens(newAppIds, newSecrets);
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer)
Expand All @@ -1144,11 +1088,9 @@ public CompletableFuture<Revision> activateToken(Author author, String appId) {
final String secret = token.secret();
assert secret != null;
final Map<String, String> newSecrets =
ImmutableMap.<String, String>builderWithExpectedSize(tokens.secrets().size() + 1)
.putAll(tokens.secrets())
.put(secret, appId).build();
addToMap(tokens.secrets(), secret, appId); // Note that the key is secret not appId.
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(), token.creation());
return new Tokens(newAppIds(tokens, appId, newToken), newSecrets);
return new Tokens(updateMap(tokens.appIds(), appId, newToken), newSecrets);
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
}
Expand All @@ -1172,10 +1114,9 @@ public CompletableFuture<Revision> deactivateToken(Author author, String appId)
assert secret != null;
final Token newToken = new Token(token.appId(), secret, token.isSystemAdmin(),
token.isSystemAdmin(), token.creation(), userAndTimestamp, null);
final Map<String, Token> newAppIds = newAppIds(tokens, appId, newToken);
final Map<String, Token> newAppIds = updateMap(tokens.appIds(), appId, newToken);
final Map<String, String> newSecrets =
tokens.secrets().entrySet().stream().filter(entry -> !entry.getKey().equals(secret))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
removeFromMap(tokens.secrets(), secret); // Note that the key is secret not appId.
return new Tokens(newAppIds, newSecrets);
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
Expand All @@ -1197,8 +1138,8 @@ public CompletableFuture<Revision> updateTokenLevel(Author author, String appId,
"The token is already " + (toBeSystemAdmin ? "admin" : "user"));
}

return new Tokens(newAppIds(tokens, appId, token.withSystemAdmin(toBeSystemAdmin)),
tokens.secrets());
final Token newToken = token.withSystemAdmin(toBeSystemAdmin);
return new Tokens(updateMap(tokens.appIds(), appId, newToken), tokens.secrets());
});
return tokenRepo.push(INTERNAL_PROJECT_DOGMA, Project.REPO_DOGMA, author, commitSummary, transformer);
}
Expand Down Expand Up @@ -1246,4 +1187,29 @@ private static void ensureProjectToken(ProjectMetadata project, String appId) {
appId + " is not a token of the project '" + project.name() + '\'');
}
}

private static <T> ImmutableMap<String, T> addToMap(Map<String, T> map, String key, T value) {
return ImmutableMap.<String, T>builderWithExpectedSize(map.size() + 1)
.putAll(map)
.put(key, value)
.build();
}

private static <T> Map<String, T> updateMap(Map<String, T> map, String key, T value) {
final ImmutableMap.Builder<String, T> builder = ImmutableMap.builderWithExpectedSize(map.size());
for (Entry<String, T> entry : map.entrySet()) {
if (entry.getKey().equals(key)) {
builder.put(key, value);
} else {
builder.put(entry);
}
}
return builder.build();
}

private static <T> ImmutableMap<String, T> removeFromMap(Map<String, T> map, String id) {
return map.entrySet().stream()
.filter(e -> !e.getKey().equals(id))
.collect(toImmutableMap(Entry::getKey, Entry::getValue));
}
}
2 changes: 1 addition & 1 deletion server/src/main/java/com/linecorp/centraldogma/server/metadata/Roles.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public int hashCode() {

@Override
public String toString() {
return MoreObjects.toStringHelper(this).omitNullValues()
return MoreObjects.toStringHelper(this)
.add("projectRoles", projectRoles)
.add("users", users)
.add("tokens", tokens)
Expand Down

0 comments on commit 75da118

Please sign in to comment.