Add JSON5 support #655
Add JSON5 support #655
Conversation
|
|
|
This looks awesome. 😆 |
ks-yim
force-pushed
the
json5-support
branch
6 times, most recently
from
a8d64e2
to
5723c68
Compare
ks-yim
requested review from
ikhoon,
jrhee17,
minwoox and
trustin
as code owners
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #655 +/- ##
============================================
+ Coverage 70.04% 70.34% +0.29%
- Complexity 3415 3458 +43
============================================
Files 349 350 +1
Lines 13482 13544 +62
Branches 1454 1466 +12
============================================
+ Hits 9444 9528 +84
+ Misses 3153 3135 -18
+ Partials 885 881 -4
... and 7 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/internal/Jackson.java
Outdated
Show resolved
Hide resolved
| @@ -65,10 +66,20 @@ | |||
| static Change<String> ofTextUpsert(String path, String text) { | |||
| requireNonNull(text, "text"); | |||
| validateFilePath(path, "path"); | |||
| if (EntryType.guessFromPath(path) == EntryType.JSON) { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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());
}
}
...
}
| @@ -65,10 +66,20 @@ | |||
| static Change<String> ofTextUpsert(String path, String text) { | |||
| requireNonNull(text, "text"); | |||
| validateFilePath(path, "path"); | |||
| if (EntryType.guessFromPath(path) == EntryType.JSON) { | |||
There was a problem hiding this comment.
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());
}
}
...
}
...java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java
Outdated
Show resolved
Hide resolved
| @@ -633,6 +671,72 @@ void editFileWithTextPatch() throws IOException { | |||
| .isEqualTo("text in some file.\n"); | |||
| } | |||
|
|
|||
| @Test | |||
| void editJson5FileWithTextPatch() throws IOException { | |||
There was a problem hiding this comment.
For now, I think text patch is fine but this feature might become awkward if we want to prettify/normalize our json5 input. (Also, I don't think anyone would want to text patch json files..)
Ideally, I think json5 should support jsonPatch instead, but we will need a parser which I don't think is very trivial.
I'm leaning more towards not allowing text patches for json5 (at least for the client side) and considering later if we receive a feature request/or we can mark it as unstable somehow)
There was a problem hiding this comment.
@jrhee17 I was thinking of supporting both APPLY_JSON_PATCH and APPLY_TEXT_PATCH for JSON5 file but if updated via APPLY_JSON_PATCH it loses all JSON5 features and degraded to ordinary JSON.
I am not sure if it is the right direction, though...
I will ping you again when I revise the PR and hope to get some advice from there..!
| } | ||
|
|
||
| @Test | ||
| void pushMirrorsJsonFileToMetaRepository() throws UnknownHostException { |
There was a problem hiding this comment.
Moved to it/MetadataPushTest.java.
| @@ -57,33 +78,94 @@ void testEncodePathPattern() { | |||
| } | |||
|
|
|||
| @Test | |||
| void pushFileToMetaRepositoryShouldFail() throws UnknownHostException { | |||
There was a problem hiding this comment.
Moved to it/MetadataPushTest.java.
| try { | ||
| return unsafeCast(Entry.ofJson(revision, entryPath, json5Text)); | ||
| } catch (JsonParseException e) { | ||
| throw new CentralDogmaException("failed to parse JSON5 content as JSON: " + json5Text); |
There was a problem hiding this comment.
nit:
| throw new CentralDogmaException("failed to parse JSON5 content as JSON: " + json5Text); | |
| throw new CentralDogmaException("failed to parse JSON5 content as JSON: " + json5Text, e); |
| @@ -1015,7 +1016,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 (maybeJson5(c.path())) { | |||
| if (maybeJson5(path)) { | ||
| jsonNode = Json5.readTree(jsonText); | ||
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode, jsonText); | ||
| } else { | ||
| jsonNode = Jackson.readTree(jsonText); | ||
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode); | ||
| } |
There was a problem hiding this comment.
It seems like we can inline the variable and return early:
| if (maybeJson5(path)) { | |
| jsonNode = Json5.readTree(jsonText); | |
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode, jsonText); | |
| } else { | |
| jsonNode = Jackson.readTree(jsonText); | |
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, jsonNode); | |
| } | |
| if (maybeJson5(path)) { | |
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, Json5.readTree(jsonText), jsonText); | |
| } | |
| return new DefaultChange<>(path, ChangeType.UPSERT_JSON, Jackson.readTree(jsonText)); | |
| @@ -69,7 +72,22 @@ | |||
| */ | |||
| public static Entry<JsonNode> ofJson(Revision revision, String path, String content) | |||
| throws JsonParseException { | |||
| return ofJson(revision, path, Jackson.readTree(content)); | |||
| return maybeJson5(path) ? new Entry<>(revision, path, EntryType.JSON, Json5.readTree(content), content) | |||
There was a problem hiding this comment.
Let's do requireNonNull(content, "content") before we use it. 😉
| import com.linecorp.centraldogma.common.Revision; | ||
| import com.linecorp.centraldogma.testing.junit.CentralDogmaExtension; | ||
|
|
||
| public class MetaRepositoryPushTest { |
There was a problem hiding this comment.
nit:
| public class MetaRepositoryPushTest { | |
| class MetaRepositoryPushTest { |
|
|
||
| // Make change to a relevant file. | ||
| final PushResult result = client.push( | ||
| dogma.project(), dogma.repo1(), Revision.HEAD, "Edit test1.json5", |
There was a problem hiding this comment.
nit:
| dogma.project(), dogma.repo1(), Revision.HEAD, "Edit test1.json5", | |
| dogma.project(), dogma.repo1(), Revision.HEAD, "Add test1.json5", |
because we are adding it? 😄
Could you also add a test case that only changes the comments of a JSON5 file?
I think we should specially treat the case not to notify the watchers.
There was a problem hiding this comment.
I think we should specially treat the case not to notify the watchers.
This is a nice idea!!
But there seems to be no easy way to detect if the actual JSON content is changed before notifying watchers.
Maybe we can add the following code at GitRepository.java:L1497 (GitRepository#notifyWatchers) but it doesn't seem to be a right choice (it looks more costly than the savings we get from not notifying watchers).
case MODIFY:
if (isJson5(entry.getOldPath())) {
try (ObjectReader reader = jGitRepository.newObjectReader()) {
final JsonNode oldJsonNode =
Json5.readTree(reader.open(entry.getOldId().toObjectId()).getBytes());
final JsonNode newJsonNode =
Json5.readTree(reader.open(entry.getNewId().toObjectId()).getBytes());
if (oldJsonNode == newJsonNode) {
break;
}
} catch (Exception e) {
// Do nothing.
}
}
/* fall-thru */There was a problem hiding this comment.
Oops, I missed this comment. 😄
I thought we could just manipulate here:
https://github.com/line/centraldogma/blob/master/server/src/main/java/com/linecorp/centraldogma/server/storage/repository/RepositoryUtil.java#L182
If it gets too complicated, let's make a separate PR for that. 😄
There was a problem hiding this comment.
RepositoryUtil.java#L182
Seems to be a good place to put this check..!
I didn't know that it eventually gets the old content from the repo..!
Let me work on it in this PR.
There was a problem hiding this comment.
And it seems we don't even need a change. It already works in a way you wanted it to be.
I've just added a test case to prove it..!
There was a problem hiding this comment.
I didn't know that it's going to work. That's awesome. 😄
| private static <T> EntryDto convert(Repository repository, Revision revision, String path, | ||
| EntryType type, @Nullable T content) { |
There was a problem hiding this comment.
We don't need <T> anymore throughout this class :
| private static <T> EntryDto convert(Repository repository, Revision revision, String path, | |
| EntryType type, @Nullable T content) { | |
| private static EntryDto convert(Repository repository, Revision revision, String path, | |
| EntryType type, @Nullable Object content) { |
| requireNonNull(entry, "entry"); | ||
| if (withContent && entry.hasContent()) { | ||
| return convert(repository, revision, entry.path(), entry.type(), entry.content()); | ||
| // TODO(ks-yim): Not sure DtoConverter having business logic and it looks hacky to receive | ||
| // queryType to handle JSON_PATH query for JSON5 files. |
There was a problem hiding this comment.
I don't have a better solution. 😅
Let's leave it as a known issue. 👍
There was a problem hiding this comment.
Not sure of 'DtoConverter' having business logic and it looks hacky to receive queryType to handle JSON_PATH query for JSON5 files.
Agreed. QueryType and isJson5() don't match with DTO. How about taking a Function rather than adding a specific logic here?
public static EntryDto convert(Repository repository, Revision revision,
Entry<?> entry, boolean withContent, Function<Entry, ?> contentConverter) {
return convert(repository, revision, entry.path(), entry.type(), contentCoverter.apply(entry));
}| final JsonNode jsonNode = Jackson.readTree(content); | ||
| changes.putIfAbsent(localPath, Change.ofJsonUpsert(localPath, jsonNode)); | ||
| final String jsonText = new String(content, StandardCharsets.UTF_8); | ||
| changes.putIfAbsent(localPath, Change.ofJsonUpsert(localPath, jsonText)); |
There was a problem hiding this comment.
How about also adding the variant method that takes byte[] so that we can remove String conversion?
static Change<JsonNode> ofJsonUpsert(String path, byte[] jsonBytes) {...}
|
@minwoox I've resolved most of your comments except the feature of not notifying watchers for any changes that does not affect the actual content of JSON5. And I added a validation before committing a patch on JSON5 files (since they are updated via text patch..). Plus, I found a weird bug with Jackson's I've summarized the issue at // String conversion is a workaround for Jackson bug with 'ALLOW_SINGLE_QUOTES' feature
// when it deserializes JSON5 from byte array source.
// If double quotes are used within a single quoted string, it either raises JsonParseException or
// removes double quotes while deserializing.
// e.g. 'I can use "double quotes" here' deserialized into "I can use double quotes here"
// 'I can use double quotes "here"' raises JsonParseException.I will report the issue to Jackson but I think we should always convert |
👍
Oops, I didn't notice that. 😅 Yeah, let's report it to the upstream and use String for now.
Could you tell me more about it, please? |
| @@ -106,6 +110,7 @@ | |||
|
|
|||
| this.path = path; | |||
| this.content = content; | |||
| this.contentAsText = contentAsText; | |||
There was a problem hiding this comment.
I wonder if we should also add json5 specific logic when performing json deserialization
which may be done when replicating logs
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| // TODO(ks-yim): Not sure of 'DtoConverter' having business logic and it looks hacky to receive | ||
| // queryType to handle JSON_PATH query for JSON5 files. | ||
| return convert(repository, revision, entry.path(), entry.type(), | ||
| isJson5(entry) && queryType != QueryType.JSON_PATH ? entry.contentAsText() |
There was a problem hiding this comment.
Q) Did we agree to move this functionality to api v2?
Since type == JSON and content is json5, I feel like this will break backwards-compatibility
There was a problem hiding this comment.
Haven't talked about V2 part, yet.
Since type == JSON and content is json5, I feel like this will break backwards-compatibility
I don't think there will be a breaking change with ArmeriaCentralDogma client unless users are using deprecated Query.identity(...) because JSON5 can now only be queried via Query.ofText(..) and this PR still supports it.
The only breaking change that I can think of is that users won't be able to upsert JSON5 text with Change.ofTextUpsert(...) after this PR on anymore, though I doubt there will be many users who do this.
But I am not sure if this PR won't break admin UI and its APIs somehow.
Let me check this out.
There was a problem hiding this comment.
Let me be more specific since I'm starting to doubt myself as well 😅 I've run the test ContentServiceV1Test#getFile to check this behavior
I was thinking of the following scenario:
- The user sends a simple file request, the entry value at this line is
Entry{revision=2, path=/foo.json5, type=JSON, content={a: 'bar'}} ArmeriaCentralDogmain this PR parses this entry no problem- Old clients or clients for other languages (i.e. python, go, etc..) can't parse this entry. At the very least, the old version java client can't parse
json5.
No problem if this scenario isn't an issue 😅 (hopefully this is just my misunderstanding 🙏 )
There was a problem hiding this comment.
I was thinking of the following scenario:
Oh, I've thought CD client's getFile API always accept Query but it seems not.
There could be a compatibility issue if a user uses the following API.
The return type of Entry#content() retrieved via this API is no longer String but TextNode for JSON5 files.
default CompletableFuture<Entry<?>> getFile(String projectName, String repositoryName,
Revision revision, String path) {
@SuppressWarnings("unchecked")
final CompletableFuture<Entry<?>> f = (CompletableFuture<Entry<?>>) (CompletableFuture<?>)
getFile(projectName, repositoryName, revision, Query.of(QueryType.IDENTITY, path));
return f;
}There was a problem hiding this comment.
Also had a chat about this and decided to go for ContentServiceV2 which will be handled in another PR.
|
I think it's almost done, excellent job, @ks-yim! |
|
@minwoox Thanks for the patience. It seems Jackson's |
I really appreciate all the efforts. Please, don't worry about it. 😄
Oops, let me also take a look at it. |
| 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); |
There was a problem hiding this comment.
nit: Could move jsonNode into try..catch block?
| final EntryType oldEntryType = !isJson5(oldPath) ? EntryType.guessFromPath(oldPath) | ||
| : EntryType.TEXT; |
There was a problem hiding this comment.
EntryType.guessFromPath() seems not to satisfy our requirements anymore.
How about introducing a new type such as FileType.
enum FileType {
JSON,
JSON5,
DIRECTORY,
TEXT,
UNKNOWN; // ??
// This method would replace all EntryType.guessFromPath() usages.
public static FileType guessFromPath(String path) {
...
}
// If there are some requirements.
EntryType toEntryType() {
...
}
}| if (!Objects.equals(newJsonNode, oldJsonNode)) { | ||
| applyPathEdit(dirCache, new InsertJson(changePath, inserter, newJsonNode)); | ||
| numEdits++; | ||
| if (!isJson5(changePath)) { |
There was a problem hiding this comment.
I'd like to minimize to use isJson5() from our codebase and make this type follow the change.type().
Question: Could we set UPSERT_TEXT to change.type() if JSON5 is specified when creating Change.ofJsonUpsert()?
| Objects.equals(content, that.content); | ||
| Objects.equals(content, that.content) && | ||
| // Compare 'contentAsText' for JSON5 entries. | ||
| (!isJson5(path) || Objects.equals(contentAsText, that.contentAsText)); |
There was a problem hiding this comment.
Could we simplify?
| (!isJson5(path) || Objects.equals(contentAsText, that.contentAsText)); | |
| Objects.equals(contentAsText, that.contentAsText); |
| requireNonNull(entry, "entry"); | ||
| if (withContent && entry.hasContent()) { | ||
| return convert(repository, revision, entry.path(), entry.type(), entry.content()); | ||
| // TODO(ks-yim): Not sure DtoConverter having business logic and it looks hacky to receive | ||
| // queryType to handle JSON_PATH query for JSON5 files. |
There was a problem hiding this comment.
Not sure of 'DtoConverter' having business logic and it looks hacky to receive queryType to handle JSON_PATH query for JSON5 files.
Agreed. QueryType and isJson5() don't match with DTO. How about taking a Function rather than adding a specific logic here?
public static EntryDto convert(Repository repository, Revision revision,
Entry<?> entry, boolean withContent, Function<Entry, ?> contentConverter) {
return convert(repository, revision, entry.path(), entry.type(), contentCoverter.apply(entry));
}| @@ -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())) { | |||
There was a problem hiding this comment.
How about adding the FileType mentioned to Change so that we check file type more statically?
public interface Change<T> {
FileType fileType() {
...
}
}
|
👀 @ks-yim Thanks for your work. I was just wondering whether this is still in progress. |
|
@jyterencekim No 😭. Stopped working on this for now. You can take this issue if you want.. Just some notes tracing back an old memory.. This PR somewhat has achieved JSON5 support in CentralDogma, but is littered with hacky workarounds and improvisations. As @ikhoon has pointed out in his suggestion of adding Although JSON5 can be parsed as a normal JSON, parsed data can't be reversed back into the original JSON5 content so the data should be treated as
|
|
Let us continue to work on this after adding API v2. @ks-yim Sorry to burden you. You are free to go. 😆 @jyterencekim Could you use a plain text file for JSON5 before we implement this? If you add the validation logic before pushing a file, I think you can use JSON5 without problems. 🤔 |
|
@minwoox thanks you for all the valuable feedback you gave while I am working on this 😄. Closing this PR in favor of API v2. |
|
What I meant was I was going to work on this PR again after adding V2. 😓 Could you reopen this? |
|
@minwoox Oops, my mistake 🤣. Reopened the PR! Looking forward to the rework! |
Motivation:
Modifications:
Server & ArmeriaCentralDogmaclient JSON5 supportgetFile()asEntry<JsonNode>but can still get the raw content via#contentAsText()watchFile()asEntry<JsonNode>but can still get the raw content via#contentAsText()getFile()&watchFile()withJSON_PATHmergeFiles()Update Admin UI's ace editor version to
1.4.13Results: