Add better YAML supports #616
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #616 +/- ##
============================================
+ Coverage 69.94% 70.05% +0.10%
+ Complexity 3399 3349 -50
============================================
Files 349 334 -15
Lines 13464 13276 -188
Branches 1451 1420 -31
============================================
- Hits 9417 9300 -117
+ Misses 3164 3106 -58
+ Partials 883 870 -13
Continue to review full report at Codecov.
|
|
This looks awesome! Thanks for creating this PR. |
| @@ -258,6 +258,12 @@ | |||
| " (expected: " + queryType + ')'); | |||
| } | |||
| return entryAsJson(query, normRev, content); | |||
| case IDENTITY_YAML: | |||
| if (receivedEntryType != com.linecorp.centraldogma.internal.thrift.EntryType.YAML) { | |||
| throw new CentralDogmaException("invalid entry type. entry type: " + receivedEntryType + | |||
There was a problem hiding this comment.
How about extracting this duplicate logic?
chacham
force-pushed
the
feature/yaml-support
branch
from
a15084a to
6299f07
Compare
|
@minwoox, I read your previous comment and continued with using SnakeYAML directly.
But, as I continue to work, using SnakeYAML's
So, I'll modify it to use Jackson instead of using SnakeYAML directly. |
I haven't known about the limitations. 😅 |
|
Since jackson-dataformat-yaml also uses the Jackson |
I think we can check if the |
The problem is |
IIRC, the |
Oh, I'll check your comment and see it again. Thank you! |
That's probably because we didn't modify the client? 😄 |
|
Do we need to add new features for the legacy Thrift client and service? 🤔 |
common/src/main/java/com/linecorp/centraldogma/common/Change.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/common/EntryType.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/common/ContentHolder.java
Outdated
Show resolved
Hide resolved
common/src/main/java/com/linecorp/centraldogma/internal/Util.java
Outdated
Show resolved
Hide resolved
| @@ -817,6 +825,7 @@ private static TreeFilter pathPatternFilterOrTreeFilter(@Nullable String pathPat | |||
| switch (diffEntry.getChangeType()) { | |||
| case MODIFY: | |||
| final EntryType oldEntryType = EntryType.guessFromPath(oldPath); | |||
| final JsonPatch patch; | |||
There was a problem hiding this comment.
If patch is not used outside of case, isn't it better to revert and minimize its scope?
There was a problem hiding this comment.
I'll revert it when when making Jackson.readTree() receive EntryType.
| final JsonNode oldYamlNode = | ||
| JacksonYaml.readTree( | ||
| reader.open(diffEntry.getOldId().toObjectId()).getBytes()); | ||
| final JsonNode newYamlNode = | ||
| JacksonYaml.readTree( | ||
| reader.open(diffEntry.getNewId().toObjectId()).getBytes()); |
There was a problem hiding this comment.
I think if we consolidate JacksonYaml and Jackson, we can remove some duplicate code.
How about taking EntryType as a parameter for readTree() and writeValueAsBytes()
class Jackson {
public static JsonNode readTree(String data, EntryType type) throws JsonParseException {
// Raise an IllegalArgumentException it is neither JSON nor YAML
if (type == YAML) {
// Directly use yamlMapper or internally use JacksonYaml
} else {
// Use compactJsonMapper
}
}
...
}There was a problem hiding this comment.
On second thought, it would be better to make Jackson interface and use like:
Jackson.ofJson().readTree(...)
Jackson.ofYaml().readTree(...)
// dynamically get the implementation
Jackson.of(YAML).readTree(...)
|
Because the |
Could you give us a specific example of the case, please? |
Especially, |
chacham
force-pushed
the
feature/yaml-support
branch
2 times, most recently
from
f1bb7e9 to
1960c21
Compare
public interface Jackson {
static ofJson() {
return JacksonJson.INSTANCE;
}
static ofYaml() {
return JacksonYaml.INSTANCE;
}
static of(EntryType type) {
if (type == JSON) {
return JacksonJson.INSTANCE;
} else if (type == YAML) {
...
}
}
T readValue(String data, Class<T> type);
...
}
abstract class AbstractJackson implements Jackson {
AbstractJackson(ObjectMapper compactMapper, ObjectMapper prettyMapper) {
...
}
@Override
public <T> T readValue(String data, Class<T> type) throws JsonParseException, JsonMappingException {
try {
return compactMapper.readValue(data, type);
} catch (JsonParseException | JsonMappingException e) {
throw e;
} catch (IOException e) {
throw new IOError(e);
}
}
// Copy all other methods from the old Jackson.java
...
}
// Could be an enum class
public final class JacksonJson extends AbstractJackson {
private static final ObjectMapper compactMapper = new ObjectMapper();
private static final ObjectMapper prettyMapper = new ObjectMapper();
private static JacksonJson INSTANCE = new JacksonJson();
static {
...
}
private JacksonJson() {
super(compactMapper, prettyMapper);
}
}
public final class JacksonYaml extends AbstractJackson {
private static final YAMLMapper yamlMapper = new YAMLMapper();
private static JacksonYaml INSTANCE = new JacksonYaml();
static {
...
}
private JacksonJson() {
// Perhaps, we don't support compact and pretty for YAML
super(yamlMapper, yamlMapper);
}
} |
Any though on this? @minwoox |
It seems there are several |
If it is more complex not to support legacy client, we can ignore the suggestion. I thought we don't put in a lot of work to legacy APIs. |
I added tests for YAML (similar to the existing JSON/TEXT), and there doesn't seem to be big problems for now. |
👍 |
|
I changed Jackson to interface and separated JSON/YAML implementation using abstract class. But because of the json path config and some methods, it looks a bit weird. |
I think it's okay! Please, go ahead! 😄 |
|
I guess this is ready so let me change the status of this PR. 😉 |
|
Let me do the final review after #616 (comment) is addressed. 🙇♂️ |
chacham
force-pushed
the
feature/yaml-support
branch
from
33ae5ac to
f2332e2
Compare
|
Old clients crashes when |
Had a chat with @ikhoon and I think it's time to go for I think there's a lot to do to use |
Supports YAML Entry and upsert YAML
chacham
force-pushed
the
feature/yaml-support
branch
from
f2332e2 to
8c2ebb1
Compare
Motivation
#151 Need better YAML supports!
This is my first time trying to contribute to an open source project.
Please let me know if I'm doing something wrong!
Modifications
client.mergeFiles()In Progress
TO-DO (Maybe later)
YAML_PATHfromJSON_PATH?