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.

Sign up for GitHub

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

Jump to bottom

Add JSON5 support #655

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open

Add JSON5 support #655

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0a070ab
JSON5 upsert
ks-yim Jan 3, 2022
96900f3
Add unit tests
ks-yim Jan 3, 2022
5ba66fe
Add JSON5 support to `ArmeriaCentralDogma`
ks-yim Jan 3, 2022
e384b9d
Add ITs
ks-yim Jan 4, 2022
b780570
Clean up
ks-yim Jan 4, 2022
ade904f
Merge master
ks-yim Jan 5, 2022
3ef8a6a
isJson5
ks-yim Jan 5, 2022
7ed6751
Resolve minwoox's comments
ks-yim Jan 5, 2022
e6d76a7
Add isJson
ks-yim Jan 5, 2022
6a5218f
Workaround for Jackson bug with 'ALLOW_SINGLE_QUOTES' feature
ks-yim Jan 6, 2022
ee64818
Merge master
ks-yim Jan 6, 2022
cc71c25
JSON5 validation upon edit
ks-yim Jan 6, 2022
2463422
typo
ks-yim Jan 6, 2022
cd45a77
Avoid string convertion in Entry#ofJson
ks-yim Jan 6, 2022
2384e54
Show server error if 'add file' fails in admin UI
ks-yim Jan 6, 2022
e0f6fbb
Add test case for watcher not notified unless JSON5 content changes
ks-yim Jan 7, 2022
a3c3e64
Normalize JSON5 upsert as text upsert to properly replicate JSON5 con…
ks-yim Feb 2, 2022
d7f23f6
Fix WatchJson5 test
ks-yim Feb 2, 2022
a8d6adb
Remove `lineBreaks` in test JSON5 to fix windows test
ks-yim Feb 21, 2022
bcaef45
Fix ResponseTimeout in unit test
ks-yim Feb 22, 2022
dc4e076
Fix `editJson5FileWithJsonPatch` test
ks-yim Feb 22, 2022
cb9ae45
Fix windows test failure due to carriage return
ks-yim Feb 22, 2022
849bdb2
Merge branch 'master' into json5-support
ikhoon Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 18 additions & 4 deletions ...a-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.linecorp.centraldogma.internal.Util.isJson5;
import static com.linecorp.centraldogma.internal.Util.unsafeCast;
import static com.linecorp.centraldogma.internal.api.v1.HttpApiV1Constants.PROJECTS_PREFIX;
import static com.linecorp.centraldogma.internal.api.v1.HttpApiV1Constants.REMOVED;
Expand Down Expand Up @@ -978,7 +979,9 @@ private static ArrayNode toJson(Iterable<? extends Change<?>> changes) {
changeNode.put("path", c.path());
changeNode.put("type", c.type().name());
final Class<?> contentType = c.type().contentType();
if (contentType == JsonNode.class) {
if (isJson5(c.path())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about adding the FileType mentioned to Change so that we check file type more statically?

public interface Change<T> {

    FileType fileType() {
         ...
    }
}

changeNode.put("content", c.contentAsText());
} else if (contentType == JsonNode.class) {
changeNode.set("content", (JsonNode) c.content());
} else if (contentType == String.class) {
changeNode.put("content", (String) c.content());
Expand Down Expand Up @@ -1032,11 +1035,11 @@ private static <T> Entry<T> toEntry(Revision revision, JsonNode node, QueryType
throw new CentralDogmaException("invalid entry type. entry type: " + receivedEntryType +
" (expected: " + queryType + ')');
}
return entryAsJson(revision, node, entryPath);
return entryAsJson(revision, node, entryPath, queryType);
case IDENTITY:
switch (receivedEntryType) {
case JSON:
return entryAsJson(revision, node, entryPath);
return entryAsJson(revision, node, entryPath, queryType);
case TEXT:
return entryAsText(revision, node, entryPath);
case DIRECTORY:
Expand All @@ -1057,7 +1060,18 @@ private static <T> Entry<T> entryAsText(Revision revision, JsonNode node, String
return unsafeCast(Entry.ofText(revision, entryPath, content0));
}

private static <T> Entry<T> entryAsJson(Revision revision, JsonNode node, String entryPath) {
private static <T> Entry<T> entryAsJson(Revision revision, JsonNode node, String entryPath,
QueryType queryType) {
// 'content' of the JSON5 identity query response is always 'TextNode' to preserve JSON5 format
// while the 'content' of JSON_PATH query can be whatever 'JsonNode'.
if (isJson5(entryPath) && queryType != QueryType.JSON_PATH) {
final String json5Text = getField(node, "content").asText();
try {
return unsafeCast(Entry.ofJson(revision, entryPath, json5Text));
} catch (JsonParseException e) {
throw new CentralDogmaException("failed to parse JSON5 content as JSON: " + json5Text, e);
}
}
return unsafeCast(Entry.ofJson(revision, entryPath, getField(node, "content")));
}

Expand Down
159 changes: 122 additions & 37 deletions ...meria/src/test/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogmaTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,55 +15,140 @@
*/
package com.linecorp.centraldogma.client.armeria;

import static java.util.concurrent.Executors.newSingleThreadScheduledExecutor;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.net.UnknownHostException;
import java.util.concurrent.CompletionException;
import static org.mockito.Mockito.when;

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;
import org.mockito.ArgumentMatchers;
import org.mockito.Mock;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.databind.JsonNode;

import com.linecorp.armeria.client.WebClient;
import com.linecorp.armeria.common.HttpResponse;
import com.linecorp.armeria.common.RequestHeaders;
import com.linecorp.centraldogma.client.CentralDogma;
import com.linecorp.centraldogma.common.Change;
import com.linecorp.centraldogma.common.InvalidPushException;
import com.linecorp.centraldogma.common.PushResult;
import com.linecorp.centraldogma.testing.junit.CentralDogmaExtension;
import com.linecorp.centraldogma.common.Entry;
import com.linecorp.centraldogma.common.Query;
import com.linecorp.centraldogma.common.Revision;
import com.linecorp.centraldogma.internal.Jackson;
import com.linecorp.centraldogma.internal.Json5;

class ArmeriaCentralDogmaTest {

@RegisterExtension
static CentralDogmaExtension dogma = new CentralDogmaExtension() {
@Override
protected void scaffold(CentralDogma client) {
client.createProject("foo").join();
}
};
private static final String JSON5_STRING =
"{\n" +
" // comments\n" +
" unquoted: 'and you can quote me on that',\n" +
" singleQuotes: 'I can use \"double quotes\" here',\n" +
" leadingDecimalPoint: .8675309," +
" trailingComma: 'in objects', andIn: ['arrays',]," +
" \"backwardsCompatible\": \"with JSON\",\n" +
"}\n";

@Mock
private WebClient webClient;

private CentralDogma client;

static <T> Entry<T> getFile(CentralDogma client, Query<T> query) {
return client.getFile("foo", "bar", Revision.INIT, query).join();
}

static <T> Entry<T> watchFile(CentralDogma client, Query<T> query) {
return client.watchFile("foo", "bar", Revision.INIT, query).join();
}

static void validateJson5Entry(Entry<?> entry) throws JsonParseException {
assertThat(entry.path()).isEqualTo("/foo.json5");
assertThat(entry.content()).isEqualTo(Json5.readTree(JSON5_STRING));
assertThat(entry.contentAsText()).isEqualTo(JSON5_STRING);
}

@BeforeEach
void setUp() {
client = new ArmeriaCentralDogma(newSingleThreadScheduledExecutor(), webClient, "access_token");
}

@Test
void testGetJson5File() throws Exception {
when(webClient.execute(ArgumentMatchers.<RequestHeaders>any())).thenReturn(
HttpResponse.ofJson(new MockEntryDto("/foo.json5", "JSON", JSON5_STRING)));

final Entry<?> entry = getFile(client, Query.ofJson("/foo.json5"));
validateJson5Entry(entry);
}

@Test
void pushFileToMetaRepositoryShouldFail() throws UnknownHostException {
Copy link
Author

Choose a reason for hiding this comment

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

Moved to it/MetadataPushTest.java.

final CentralDogma client = new ArmeriaCentralDogmaBuilder()
.host(dogma.serverAddress().getHostString(), dogma.serverAddress().getPort())
.build();

assertThatThrownBy(() -> client.forRepo("foo", "meta")
.commit("summary", Change.ofJsonUpsert("/bar.json", "{ \"a\": \"b\" }"))
.push()
.join())
.isInstanceOf(CompletionException.class)
.hasCauseInstanceOf(InvalidPushException.class);
void testGetJson5FileWithJsonPath() throws Exception {
final JsonNode node = Jackson.readTree("{\"a\": \"bar\"}");
when(webClient.execute(ArgumentMatchers.<RequestHeaders>any())).thenReturn(
HttpResponse.ofJson(new MockEntryDto("/foo.json5", "JSON", node)));

final Entry<?> entry = getFile(client, Query.ofJsonPath("/foo.json5", "$.a"));
assertThat(entry.content()).isEqualTo(node);
}

@Test
void pushMirrorsJsonFileToMetaRepository() throws UnknownHostException {
Copy link
Author

Choose a reason for hiding this comment

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

Moved to it/MetadataPushTest.java.

final CentralDogma client = new ArmeriaCentralDogmaBuilder()
.host(dogma.serverAddress().getHostString(), dogma.serverAddress().getPort())
.build();

final PushResult result = client.forRepo("foo", "meta")
.commit("summary", Change.ofJsonUpsert("/mirrors.json", "[]"))
.push()
.join();
assertThat(result.revision().major()).isPositive();
void testWatchJson5File() throws Exception {
final MockEntryDto entryDto = new MockEntryDto("/foo.json5", "JSON", JSON5_STRING);
when(webClient.execute(ArgumentMatchers.<RequestHeaders>any())).thenReturn(
HttpResponse.ofJson(new MockWatchResultDto(1, entryDto)));

final Entry<?> entry = watchFile(client, Query.ofJson("/foo.json5"));
validateJson5Entry(entry);
}

static class MockEntryDto {

private final String path;
private final String type;
private final Object content;

MockEntryDto(String path, String type, Object content) {
this.path = path;
this.type = type;
this.content = content;
}

@JsonProperty("path")
String path() {
return path;
}

@JsonProperty("type")
String type() {
return type;
}

@JsonProperty("content")
Object content() {
return content;
}
}

static class MockWatchResultDto {

private final int revision;
private final MockEntryDto entry;

MockWatchResultDto(int revision, MockEntryDto entry) {
this.revision = revision;
this.entry = entry;
}

@JsonProperty("revision")
int revision() {
return revision;
}

@JsonProperty("entry")
MockEntryDto entry() {
return entry;
}
}
}
51 changes: 41 additions & 10 deletions common/src/main/java/com/linecorp/centraldogma/common/Change.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package com.linecorp.centraldogma.common;

import static com.linecorp.centraldogma.internal.Util.isJson5;
import static com.linecorp.centraldogma.internal.Util.validateDirPath;
import static com.linecorp.centraldogma.internal.Util.validateFilePath;
import static java.util.Objects.requireNonNull;
Expand All @@ -34,10 +35,12 @@
import javax.annotation.Nullable;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonParseException;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;

import com.linecorp.centraldogma.internal.Jackson;
import com.linecorp.centraldogma.internal.Json5;
import com.linecorp.centraldogma.internal.Util;
import com.linecorp.centraldogma.internal.jsonpatch.JsonPatch;
import com.linecorp.centraldogma.internal.jsonpatch.ReplaceMode;
Expand All @@ -51,6 +54,22 @@
@JsonDeserialize(as = DefaultChange.class)
public interface Change<T> {

/**
* Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_TEXT}.
*
* <p>Note that you should use {@link #ofJsonUpsert(String, String)} if the specified {@code path} ends with
* {@code ".json"}. The {@link #ofJsonUpsert(String, String)} will check that the given {@code text} is a
* valid JSON.
*
* @param path the path of the file
* @param content UTF-8 encoded text file content
* @throws ChangeFormatException if the path ends with {@code ".json"}
*/
static Change<String> ofTextUpsert(String path, byte[] content) {
requireNonNull(content, "content");
return ofTextUpsert(path, new String(content, StandardCharsets.UTF_8));
}

/**
* Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_TEXT}.
*
Expand All @@ -65,13 +84,26 @@ public interface Change<T> {
static Change<String> ofTextUpsert(String path, String text) {
requireNonNull(text, "text");
validateFilePath(path, "path");
if (EntryType.guessFromPath(path) == EntryType.JSON) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems not to make sense to use ofTextUpsert for not JSON but JSON5.
How about modifying Change.ofJsonUpsert(String path, String jsonText) to take JSON5?

static Change<JsonNode> ofJsonUpsert(String path, String jsonText) {
  
    final JsonNode jsonNode = ...;
   
    return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode, jsonText);
}

I think we can give jsonText a higher priority to serialize the data.

Copy link
Contributor

@jrhee17 jrhee17 Dec 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just wanted to point out we may need to use contentAsText conditionally for serialization right before sending on wire. It's not elegant, but I think this is worth it considering the gain 👍

ArmeriaCentralDogma.java

private static ArrayNode toJson(Iterable<? extends Change<?>> changes) {
...
    if (c.path().endsWith(".json5")) {
        changeNode.put("content", c.contentAsText());
    } else {
        final Class<?> contentType = c.type().contentType();
        if (contentType == JsonNode.class) {
            changeNode.set("content", (JsonNode) c.content());
        } else if (contentType == String.class) {
            changeNode.put("content", (String) c.content());
        }
    }
...
}

if (EntryType.guessFromPath(path) == EntryType.JSON && !isJson5(path)) {
throw new ChangeFormatException("invalid file type: " + path +
" (expected: a non-JSON file). Use Change.ofJsonUpsert() instead");
}
return new DefaultChange<>(path, ChangeType.UPSERT_TEXT, text);
}

/**
* Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_JSON}.
*
* @param path the path of the file
* @param jsonContent the content of the file
*
* @throws ChangeFormatException if the specified {@code jsonText} is not a valid JSON
*/
static Change<JsonNode> ofJsonUpsert(String path, byte[] jsonContent) {
requireNonNull(jsonContent, "jsonContent");
return ofJsonUpsert(path, new String(jsonContent, StandardCharsets.UTF_8));
}

/**
* Returns a newly-created {@link Change} whose type is {@link ChangeType#UPSERT_JSON}.
*
Expand All @@ -85,12 +117,15 @@ static Change<JsonNode> ofJsonUpsert(String path, String jsonText) {

final JsonNode jsonNode;
try {
if (isJson5(path)) {
jsonNode = Json5.readTree(jsonText);
return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode, jsonText);
}
jsonNode = Jackson.readTree(jsonText);
} catch (IOException e) {
return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode);
Comment on lines 118 to +125
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Could move jsonNode into try..catch block?

} catch (JsonParseException e) {
minwoox marked this conversation as resolved.
Show resolved Hide resolved
throw new ChangeFormatException("failed to read a value as a JSON tree", e);
}

return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode);
}

/**
Expand Down Expand Up @@ -140,10 +175,6 @@ static Change<String> ofRename(String oldPath, String newPath) {
static Change<String> ofTextPatch(String path, @Nullable String oldText, String newText) {
validateFilePath(path, "path");
requireNonNull(newText, "newText");
if (EntryType.guessFromPath(path) == EntryType.JSON) {
throw new ChangeFormatException("invalid file type: " + path +
" (expected: a non-JSON file). Use Change.ofJsonPatch() instead");
}

final List<String> oldLineList = oldText == null ? Collections.emptyList()
: Util.stringToLines(oldText);
Expand All @@ -152,7 +183,7 @@ static Change<String> ofTextPatch(String path, @Nullable String oldText, String
final Patch<String> patch = DiffUtils.diff(oldLineList, newLineList);
final List<String> unifiedDiff = DiffUtils.generateUnifiedDiff(path, path, oldLineList, patch, 3);

return new DefaultChange<>(path, ChangeType.APPLY_TEXT_PATCH, String.join("\n", unifiedDiff));
return ofTextPatch(path, String.join("\n", unifiedDiff));
}

/**
Expand All @@ -170,7 +201,7 @@ static Change<String> ofTextPatch(String path, @Nullable String oldText, String
static Change<String> ofTextPatch(String path, String textPatch) {
validateFilePath(path, "path");
requireNonNull(textPatch, "textPatch");
if (EntryType.guessFromPath(path) == EntryType.JSON) {
if (EntryType.guessFromPath(path) == EntryType.JSON && !isJson5(path)) {
throw new ChangeFormatException("invalid file type: " + path +
" (expected: a non-JSON file). Use Change.ofJsonPatch() instead");
}
Expand Down
5 changes: 5 additions & 0 deletions common/src/main/java/com/linecorp/centraldogma/common/DefaultChange.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,10 @@ private static DefaultChange<?> rejectIncompatibleContent(@Nullable JsonNode con
private String contentAsText;

DefaultChange(String path, ChangeType type, @Nullable T content) {
this(path, type, content, null);
}

DefaultChange(String path, ChangeType type, @Nullable T content, @Nullable String contentAsText) {
this.type = requireNonNull(type, "type");

if (type.contentType() == JsonNode.class) {
Expand All @@ -106,6 +110,7 @@ private static DefaultChange<?> rejectIncompatibleContent(@Nullable JsonNode con

this.path = path;
this.content = content;
this.contentAsText = contentAsText;
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should also add json5 specific logic when performing json deserialization

centraldogma/common/src/main/java/com/linecorp/centraldogma/common/DefaultChange.java

Lines 38 to 40 in 3642633

static DefaultChange<?> deserialize(@JsonProperty("type") ChangeType type,
@JsonProperty("path") String path,
@JsonProperty("content") @Nullable JsonNode content) {

which may be done when replicating logs

centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/replication/ZooKeeperCommandExecutor.java

Line 1034 in 3642633

final ReplicationLog<?> log = Jackson.readValue(bytes, ReplicationLog.class);

Copy link
Author

Choose a reason for hiding this comment

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

Hmm.. thanks for pointing this out. I think we should handle the case that Change#ofJsonUpsert is used to add a new JSON5 file(ChangeType.ADD) as it relies on contentAsText() which isn't serialized into replication log.

No need to care about Change#ofJsonUpsert to modify the file content(ChangeType.MODIFY) since it internally converted into Change.ofTextPatch.

I think we should have Change#contentAsText in the serialized content to make it work (or a simple workaround would be internally converting Change#ofJsonUpsert to Change#ofTextUpsert like we did for ChangeType.MODIFY).

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about the late reply 😅

I wonder if it's difficult to tweak behavior so that serde happens in the same way as we've been doing so far (using Change#ofJsonUpsert(String path, String jsonText)).

I think we should have Change#contentAsText in the serialized content to make it work

I personally think we don't want contentAsText for all file types. If we want to selectively include contentAsText depending on the file path, I also think the increased complexity would be similar to the other idea.

Let me know if you feel differently though 😅

Copy link
Author

@ks-yim ks-yim Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wonder if it's difficult to tweak behavior so that serde happens in the same way as we've been doing so far

The type of content in Change#ofJsonUpsert is JsonNode because of the decision we've made for JSON5 entry type (use JsonNode over String).

Maybe we can do the tweak that Change to a JSON5 file have TextNode as its content and do if-else in all places where change gets applied, but this also seems to me extremely unmanageable.

I agree that we should avoid unnecessary complexity, but either way(tweaking serde vs tweak `Change) is not very appealing 😅..

The 3rd(and the most simple) option would be converting Change#ofJsonUpsert request to Change#ofTextUpsert internally like we already did for ChangeType.MODIFY.

Copy link
Author

@ks-yim ks-yim Jan 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It turns out that the 3rd option will never work since the serializing and sending replication log happens earlier than the point that we can do conversion.

Copy link
Author

@ks-yim ks-yim Feb 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Had a chat with @minwoox and @jrhee17 about this and I was advised to put denormalized JSON5 Change to replication logs to preserve its original content.

I tried this approach only to find there's no easy way to pass the original JSON5 content to replication log even with this. NormalizingPushCommand#changes gives the original JSON5 content in it, but once it gets JSON-serialized for replication log it loses its JSON5 content since contentAsText is ignored in the serialization process.

Meanwhile, it turned out that 3rd option that I mentioned above works just fine.
It works by normalizing JSON5 Change#ofJsonUpsert to Change#ofTextUpsert and then putting the normalized change to replication log (refer to GitRepository.java:L803-804).

}

@Override
Expand Down
Loading