Skip to content

Commit e072bcc

Browse files
trustintrustin
authored
Add revision property to EntryDto (#378)
Motivation: We currently do not return the revision of an entry. The reasoning behind this behavior is that the response would contain duplicate 'revision' fields if a user requested more than one entry. Because of that, there's a problem that a user could not notice quickly which version of a file is loaded into his/her service. Modifications: - Added `revision` property to `EntryDto` - Miscellaneous: - Used `HttpHeaderNames.PREFER` which reappeared in Armeria 0.83.0 - Used `HttpData.toStringUtf8()` instead of `.content().toStringUtf8()` Result: - Closes #310
1 parent 4dfacf4 commit e072bcc

File tree

19 files changed

+146
-143
lines changed

19 files changed

+146
-143
lines changed

‎client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,8 @@
9999
import com.linecorp.centraldogma.internal.Util;
100100
import com.linecorp.centraldogma.internal.api.v1.WatchTimeout;
101101

102-
import io.netty.util.AsciiString;
103-
104102
final class ArmeriaCentralDogma extends AbstractCentralDogma {
105103

106-
// TODO(trustin): Replace with HttpHeaderNames.PREFER.
107-
private static final AsciiString HEADER_NAME_PREFER = HttpHeaderNames.of("prefer");
108-
109104
private static final MediaType JSON_PATCH_UTF8 = MediaType.JSON_PATCH.withCharset(StandardCharsets.UTF_8);
110105

111106
private static final byte[] UNREMOVE_PATCH = toBytes(JsonNodeFactory.instance.arrayNode(1).add(
@@ -386,6 +381,7 @@ public <T> CompletableFuture<Entry<T>> getFile(String projectName, String reposi
386381
requireNonNull(revision, "revision");
387382
requireNonNull(query, "query");
388383

384+
// TODO(trustin) No need to normalize a revision once server response contains it.
389385
return normalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> {
390386
final StringBuilder path = pathBuilder(projectName, repositoryName);
391387
path.append("/contents").append(query.path());
@@ -418,6 +414,7 @@ public CompletableFuture<Map<String, Entry<?>>> getFiles(String projectName, Str
418414
requireNonNull(revision, "revision");
419415
validatePathPattern(pathPattern, "pathPattern");
420416

417+
// TODO(trustin) No need to normalize a revision once server response contains it.
421418
return normalizeRevision(projectName, repositoryName, revision).thenCompose(normRev -> {
422419
final StringBuilder path = pathBuilder(projectName, repositoryName);
423420
path.append("/contents");
@@ -812,7 +809,7 @@ private <T> CompletableFuture<T> watch(Revision lastKnownRevision, long timeoutM
812809
String path, Function<AggregatedHttpMessage, T> func) {
813810
final HttpHeaders headers = headers(HttpMethod.GET, path)
814811
.set(HttpHeaderNames.IF_NONE_MATCH, lastKnownRevision.text())
815-
.set(HEADER_NAME_PREFER, "wait=" + LongMath.saturatedAdd(timeoutMillis, 999) / 1000L);
812+
.set(HttpHeaderNames.PREFER, "wait=" + LongMath.saturatedAdd(timeoutMillis, 999) / 1000L);
816813

817814
try (SafeCloseable ignored = Clients.withContextCustomizer(ctx -> {
818815
ctx.setResponseTimeoutMillis(

‎common/src/main/java/com/linecorp/centraldogma/internal/api/v1/EntryDto.java‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,13 @@
2929
import com.google.common.base.MoreObjects;
3030

3131
import com.linecorp.centraldogma.common.EntryType;
32+
import com.linecorp.centraldogma.common.Revision;
3233

3334
@JsonInclude(Include.NON_NULL)
3435
public class EntryDto<T> {
3536

37+
private final Revision revision;
38+
3639
private final String path;
3740

3841
private final EntryType type;
@@ -41,30 +44,37 @@ public class EntryDto<T> {
4144

4245
private final String url;
4346

44-
public EntryDto(String path, EntryType type, String projectName, String repoName, @Nullable T content) {
47+
public EntryDto(Revision revision, String path, EntryType type,
48+
String projectName, String repoName, @Nullable T content) {
49+
this.revision = requireNonNull(revision, "revision");
4550
this.path = requireNonNull(path, "path");
4651
this.type = requireNonNull(type, "type");
4752
this.content = content;
4853
url = PROJECTS_PREFIX + '/' + projectName + REPOS + '/' + repoName + CONTENTS + path;
4954
}
5055

51-
@JsonProperty("path")
56+
@JsonProperty
57+
public Revision revision() {
58+
return revision;
59+
}
60+
61+
@JsonProperty
5262
public String path() {
5363
return path;
5464
}
5565

56-
@JsonProperty("type")
66+
@JsonProperty
5767
public EntryType type() {
5868
return type;
5969
}
6070

61-
@JsonProperty("content")
71+
@JsonProperty
6272
@Nullable
6373
public T content() {
6474
return content;
6575
}
6676

67-
@JsonProperty("url")
77+
@JsonProperty
6878
@Nullable
6979
public String url() {
7080
return url;
@@ -73,6 +83,7 @@ public String url() {
7383
@Override
7484
public String toString() {
7585
return MoreObjects.toStringHelper(this).omitNullValues()
86+
.add("revision", revision)
7687
.add("path", path)
7788
.add("type", type)
7889
.add("content", content).toString();

‎server-auth/saml/src/test/java/com/linecorp/centraldogma/server/auth/saml/SamlAuthTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public void shouldUseBuiltinWebPageOnlyForLogout() throws Exception {
7878
resp = rule.httpClient().get(AuthProvider.LOGIN_PATH).aggregate().join();
7979
assertThat(resp.status()).isEqualTo(HttpStatus.OK);
8080
assertThat(resp.headers().contentType()).isEqualTo(MediaType.HTML_UTF_8);
81-
assertThat(resp.content().toStringUtf8()).contains("<input type=\"hidden\" name=\"SAMLRequest\"");
81+
assertThat(resp.contentUtf8()).contains("<input type=\"hidden\" name=\"SAMLRequest\"");
8282

8383
// Redirect to built-in web logout page.
8484
resp = rule.httpClient().get(AuthProvider.LOGOUT_PATH).aggregate().join();
@@ -97,7 +97,7 @@ public void shouldReturnMetadata() {
9797

9898
// Check ACS URLs for the service provider.
9999
final int port = rule.serverAddress().getPort();
100-
assertThat(resp.content().toStringUtf8())
100+
assertThat(resp.contentUtf8())
101101
.contains("entityID=\"test-sp\"")
102102
.contains("<md:AssertionConsumerService " +
103103
"Binding=\"urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST\" " +

‎server-auth/shiro/src/main/java/com/linecorp/centraldogma/server/auth/shiro/LoginService.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -165,9 +165,8 @@ private UsernamePasswordToken usernamePassword(ServiceRequestContext ctx, Aggreg
165165
"The content type of a login request must be '%s'.", MediaType.FORM_DATA);
166166
}
167167

168-
final Map<String, List<String>> parameters = new QueryStringDecoder(
169-
req.content().toStringUtf8(), false).parameters();
170-
168+
final Map<String, List<String>> parameters = new QueryStringDecoder(req.contentUtf8(),
169+
false).parameters();
171170
// assume that the grant_type is "password"
172171
final List<String> usernames = parameters.get("username");
173172
final List<String> passwords = parameters.get("password");

‎server-auth/shiro/src/test/java/com/linecorp/centraldogma/server/auth/shiro/ShiroLoginAndLogoutTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ private void loginAndLogout(AggregatedHttpMessage loginRes) throws Exception {
7373
assertThat(loginRes.status()).isEqualTo(HttpStatus.OK);
7474

7575
// Ensure authorization works.
76-
final AccessToken accessToken = Jackson.readValue(loginRes.content().toStringUtf8(), AccessToken.class);
76+
final AccessToken accessToken = Jackson.readValue(loginRes.contentUtf8(), AccessToken.class);
7777
final String sessionId = accessToken.accessToken();
7878

7979
assertThat(usersMe(client, sessionId).status()).isEqualTo(HttpStatus.OK);

‎server/src/main/java/com/linecorp/centraldogma/server/internal/admin/service/RepositoryService.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ public HttpResponse deleteFile(@Param("projectName") String projectName,
151151
ServiceRequestContext ctx) {
152152
final CommitMessageDto commitMessage;
153153
try {
154-
final JsonNode node = Jackson.readTree(message.content().toStringUtf8());
154+
final JsonNode node = Jackson.readTree(message.contentUtf8());
155155
commitMessage = Jackson.convertValue(node.get("commitMessage"), CommitMessageDto.class);
156156
} catch (IOException e) {
157157
throw new IllegalArgumentException("invalid data to be parsed", e);
@@ -242,7 +242,7 @@ private CompletableFuture<?> push0(String projectName, String repoName,
242242

243243
private static Entry<CommitMessageDto, Change<?>> commitMessageAndChange(AggregatedHttpMessage message) {
244244
try {
245-
final JsonNode node = Jackson.readTree(message.content().toStringUtf8());
245+
final JsonNode node = Jackson.readTree(message.contentUtf8());
246246
final CommitMessageDto commitMessage =
247247
Jackson.convertValue(node.get("commitMessage"), CommitMessageDto.class);
248248
final EntryDto file = Jackson.convertValue(node.get("file"), EntryDto.class);

‎server/src/main/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1.java‎

Lines changed: 12 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -101,17 +101,18 @@ public CompletableFuture<List<EntryDto<?>>> listFiles(@Param("path") String path
101101
@Param("revision") @Default("-1") String revision,
102102
Repository repository) {
103103
final String normalizedPath = normalizePath(path);
104+
final Revision normalizedRev = repository.normalizeNow(new Revision(revision));
104105
final CompletableFuture<List<EntryDto<?>>> future = new CompletableFuture<>();
105-
listFiles(repository, normalizedPath, new Revision(revision), false, future);
106+
listFiles(repository, normalizedPath, normalizedRev, false, future);
106107
return future;
107108
}
108109

109-
private static void listFiles(Repository repository, String pathPattern, Revision revision,
110+
private static void listFiles(Repository repository, String pathPattern, Revision normalizedRev,
110111
boolean withContent, CompletableFuture<List<EntryDto<?>>> result) {
111112
final Map<FindOption<?>, ?> options = withContent ? FindOptions.FIND_ALL_WITH_CONTENT
112113
: FindOptions.FIND_ALL_WITHOUT_CONTENT;
113114

114-
repository.find(revision, pathPattern, options).handle((entries, thrown) -> {
115+
repository.find(normalizedRev, pathPattern, options).handle((entries, thrown) -> {
115116
if (thrown != null) {
116117
result.completeExceptionally(thrown);
117118
return null;
@@ -121,10 +122,10 @@ private static void listFiles(Repository repository, String pathPattern, Revisio
121122
// This is called once at most, because the pathPattern is not a valid file path anymore.
122123
if (isValidFilePath(pathPattern) && entries.size() == 1 &&
123124
entries.values().iterator().next().type() == DIRECTORY) {
124-
listFiles(repository, pathPattern + "/*", revision, withContent, result);
125+
listFiles(repository, pathPattern + "/*", normalizedRev, withContent, result);
125126
} else {
126127
result.complete(entries.values().stream()
127-
.map(entry -> convert(repository, entry, withContent))
128+
.map(entry -> convert(repository, normalizedRev, entry, withContent))
128129
.collect(toImmutableList()));
129130
}
130131
return null;
@@ -238,15 +239,17 @@ public CompletableFuture<?> getFiles(
238239
return watchRepository(repository, lastKnownRevision, normalizedPath, timeOutMillis);
239240
}
240241

242+
final Revision normalizedRev = repository.normalizeNow(new Revision(revision));
241243
if (query.isPresent()) {
242244
// get a file
243-
return repository.get(new Revision(revision), query.get())
244-
.handle(returnOrThrow((Entry<?> result) -> convert(repository, result, true)));
245+
return repository.get(normalizedRev, query.get())
246+
.handle(returnOrThrow((Entry<?> result) -> convert(repository, normalizedRev,
247+
result, true)));
245248
}
246249

247250
// get files
248251
final CompletableFuture<List<EntryDto<?>>> future = new CompletableFuture<>();
249-
listFiles(repository, normalizedPath, new Revision(revision), true, future);
252+
listFiles(repository, normalizedPath, normalizedRev, true, future);
250253
return future;
251254
}
252255

@@ -257,7 +260,7 @@ private CompletableFuture<?> watchFile(Repository repository, Revision lastKnown
257260

258261
return future.thenApply(entry -> {
259262
final Revision revision = entry.revision();
260-
final EntryDto<?> entryDto = convert(repository, entry, true);
263+
final EntryDto<?> entryDto = convert(repository, revision, entry, true);
261264
return (Object) new WatchResultDto(revision, entryDto);
262265
}).exceptionally(ContentServiceV1::handleWatchFailure);
263266
}

‎server/src/main/java/com/linecorp/centraldogma/server/internal/api/DtoConverter.java‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -58,23 +58,29 @@ public static RepositoryDto convert(Repository repository) {
5858
repository.creationTimeMillis());
5959
}
6060

61-
public static <T> EntryDto<T> convert(Repository repository, Entry<T> entry, boolean withContent) {
61+
public static <T> EntryDto<T> convert(Repository repository, Revision revision,
62+
Entry<T> entry, boolean withContent) {
6263
requireNonNull(entry, "entry");
6364
if (withContent && entry.hasContent()) {
64-
return convert(repository, entry.path(), entry.type(), entry.content());
65+
return convert(repository, revision, entry.path(), entry.type(), entry.content());
6566
}
66-
return convert(repository, entry.path(), entry.type());
67+
return convert(repository, revision, entry.path(), entry.type());
6768
}
6869

69-
public static <T> EntryDto<T> convert(Repository repository, String path, EntryType type) {
70-
return convert(repository, path, type, null);
70+
private static <T> EntryDto<T> convert(Repository repository, Revision revision,
71+
String path, EntryType type) {
72+
return convert(repository, revision, path, type, null);
7173
}
7274

73-
public static <T> EntryDto<T> convert(Repository repository, String path, EntryType type,
74-
@Nullable T content) {
75+
private static <T> EntryDto<T> convert(Repository repository, Revision revision, String path,
76+
EntryType type, @Nullable T content) {
7577
requireNonNull(repository, "repository");
76-
return new EntryDto<>(requireNonNull(path, "path"), requireNonNull(type, "type"),
77-
repository.parent().name(), repository.name(), content);
78+
return new EntryDto<>(requireNonNull(revision, "revision"),
79+
requireNonNull(path, "path"),
80+
requireNonNull(type, "type"),
81+
repository.parent().name(),
82+
repository.name(),
83+
content);
7884
}
7985

8086
public static PushResultDto convert(Revision revision, long commitTimeMillis) {

‎server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/WatchRequestConverter.java‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@
2222
import java.util.Optional;
2323
import java.util.concurrent.TimeUnit;
2424

25-
import com.google.common.annotations.VisibleForTesting;
2625
import com.google.common.base.MoreObjects;
2726

2827
import com.linecorp.armeria.common.AggregatedHttpMessage;
@@ -32,18 +31,12 @@
3231
import com.linecorp.centraldogma.common.Revision;
3332
import com.linecorp.centraldogma.internal.api.v1.WatchTimeout;
3433

35-
import io.netty.util.AsciiString;
36-
3734
/**
3835
* A request converter that converts to {@link WatchRequest} when the request contains
3936
* {@link HttpHeaderNames#IF_NONE_MATCH}.
4037
*/
4138
public final class WatchRequestConverter implements RequestConverterFunction {
4239

43-
// TODO(trustin): Replace with HttpHeaderNames.PREFER.
44-
@VisibleForTesting
45-
static final AsciiString HEADER_NAME_PREFER = HttpHeaderNames.of("prefer");
46-
4740
private static final long DEFAULT_TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(120);
4841

4942
/**
@@ -56,7 +49,7 @@ public Object convertRequest(ServiceRequestContext ctx, AggregatedHttpMessage re
5649
final String ifNoneMatch = request.headers().get(HttpHeaderNames.IF_NONE_MATCH);
5750
if (!isNullOrEmpty(ifNoneMatch)) {
5851
final Revision lastKnownRevision = new Revision(ifNoneMatch);
59-
final String prefer = request.headers().get(HEADER_NAME_PREFER);
52+
final String prefer = request.headers().get(HttpHeaderNames.PREFER);
6053
final long timeoutMillis;
6154
if (!isNullOrEmpty(prefer)) {
6255
timeoutMillis = getTimeoutMillis(prefer);

‎server/src/test/java/com/linecorp/centraldogma/server/auth/ReplicatedLoginAndLogoutTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public void loginAndLogout() throws Exception {
153153
assertThat(replicationLogCount()).isEqualTo(baselineReplicationLogCount + 1);
154154

155155
// Ensure authorization works at the 2nd replica.
156-
final AccessToken accessToken = Jackson.readValue(loginRes.content().toStringUtf8(), AccessToken.class);
156+
final AccessToken accessToken = Jackson.readValue(loginRes.contentUtf8(), AccessToken.class);
157157
final String sessionId = accessToken.accessToken();
158158
await().pollDelay(Duration.TWO_HUNDRED_MILLISECONDS)
159159
.pollInterval(Duration.ONE_SECOND)

0 commit comments

Comments
 (0)