Add better YAML supports #616

chacham · 2021-07-06T13:21:23Z

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

Upsert YAML
Patching YAML
Querying YAML
Implement YAML Entry
Client-side supports
- get / querying file
- Merge YAML files from client.mergeFiles()
Retrieving YAML as JSON
- YAML entry uses JsonNode
Block parsing YAML with duplicate key to prevent silent overwrites
- Tried to merge duplicate key entries, but Jackson doesn't support it.

In Progress

Basic function test with old clients
- Because of new EntryType(YAML), old clients not work properly

TO-DO (Maybe later)

Including YAML from YAML
Determine how to handle multi document YAML
Extract YAML_PATH from JSON_PATH?
Decide whether to use YAML format in YAML_PATCH (same as json patch, json format for now)

CLAassistant · 2021-07-06T13:21:29Z

All committers have signed the CLA.

codecov · 2021-07-06T13:58:24Z

Codecov Report

Merging #616 (f2332e2) into master (3642633) will increase coverage by 0.10%.
The diff coverage is 75.59%.

❗ Current head f2332e2 differs from pull request most recent head 8c2ebb1. Consider uploading reports for the commit 8c2ebb1 to get more accurate results

@@             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

Impacted Files	Coverage Δ
...om/linecorp/centraldogma/client/ClientProfile.java	`61.53% <0.00%> (ø)`
...a/com/linecorp/centraldogma/common/MergeQuery.java	`46.15% <0.00%> (-8.40%)`	⬇️
...traldogma/server/auth/saml/SamlAuthProperties.java	`84.48% <0.00%> (ø)`
.../linecorp/centraldogma/server/auth/AuthConfig.java	`52.63% <0.00%> (ø)`
...rver/internal/admin/service/RepositoryService.java	`5.63% <0.00%> (-0.09%)`	⬇️
...ernal/admin/util/RestfulJsonResponseConverter.java	`13.33% <0.00%> (-0.96%)`	⬇️
...ldogma/server/internal/api/MetadataApiService.java	`8.43% <0.00%> (-0.21%)`	⬇️
...corp/centraldogma/server/metadata/ProjectRole.java	`40.00% <0.00%> (ø)`
...om/linecorp/centraldogma/common/DefaultChange.java	`66.17% <33.33%> (-7.27%)`	⬇️
...p/centraldogma/internal/thrift/EntryConverter.java	`46.15% <42.85%> (-3.85%)`	⬇️
... and 105 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3642633...8c2ebb1. Read the comment docs.

minwoox · 2021-07-07T01:33:03Z

This looks awesome! Thanks for creating this PR.
Let us take a look at it soon. 😉
Meanwhile, please feel free to let us know if you have any questions.

build.gradle

minwoox

Looks good so far. 😉
Let us know when this is ready. 😄

minwoox · 2021-07-12T06:12:36Z

...legacy/src/main/java/com/linecorp/centraldogma/client/armeria/legacy/LegacyCentralDogma.java

@@ -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 +


How about extracting this duplicate logic?

chacham · 2021-07-18T11:47:31Z

@minwoox, I read your previous comment and continued with using SnakeYAML directly.

If so and Jackson uses SnakeYAML anyway, I think it's okay to use SnakeYAML directly unless Jackson provides better usability. 😅

But, as I continue to work, using SnakeYAML's Node seems to have several limitations.

Unlike Jackson's JsonNode, even if a SnakeYAML Node has the same value (content), it becomes equal only when it is the same Node -> It is difficult to compare YAML content, and need to change implementations a lot such as Entry.
Although it is not a problem specific to SnakeYAML, there is no standardized patch/query syntax. So, I think using jsonpatch/jsonqurey is good for now. (And it would be easy with jackson-dataformat-yaml)

So, I'll modify it to use Jackson instead of using SnakeYAML directly.
If you have any opinion, please tell me. 😅

minwoox · 2021-07-19T02:07:31Z

So, I'll modify it to use Jackson instead of using SnakeYAML directly.
If you have any opinion, please tell me.

I haven't known about the limitations. 😅
Yeah, jackson-dataformat-yaml seems a good choice for now. 😄

chacham · 2021-07-27T13:25:45Z

Since jackson-dataformat-yaml also uses the Jackson JsonNode class, there is quite a lot of duplicate code. For now, I'll focus on implementation, and I'll organize it later. 😅
In ContentHolder.contentAsText(), if content instanceof JsonNode, format content as JSON. (Even if the content is YAML actually) I am thinking about how to handle it.

minwoox · 2021-07-28T05:41:18Z

In ContentHolder.contentAsText(), if content instanceof JsonNode, format content as JSON. (Even if the content is YAML actually) I am thinking about how to handle it.

I think we can check if the EntryType is YAML in contentAsText() so that we can produce the YAML text properly. (We probably need to refactor Entry a bit.)

chacham · 2021-07-28T06:23:33Z

I think we can check if the EntryType is YAML in contentAsText() so that we can produce the YAML text properly. (We probably need to refactor Entry a bit.)

The problem is Query.ofText. With Query.ofText, the EntryType will set as TEXT, so it can't be checked now :(

minwoox · 2021-07-28T06:48:12Z

The problem is Query.ofText. With Query.ofText, the EntryType will set as TEXT, so it can't be checked now :(

IIRC, the EntryType is decided by the file extension. https://github.com/line/centraldogma/blob/master/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java#L494
Then, the fetched Entry (with the EntryType) is transferred to the client and client converts the content using the query type. https://github.com/line/centraldogma/blob/master/client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java#L463-L464
At this point, we know if the Entry is pure text or YAML so that we can convert the content as text properly.
Did I miss something? 🤔

chacham · 2021-07-28T07:19:43Z

IIRC, the EntryType is decided by the file extension. https://github.com/line/centraldogma/blob/master/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java#L494
Then, the fetched Entry (with the EntryType) is transferred to the client and client converts the content using the query type. https://github.com/line/centraldogma/blob/master/client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java#L463-L464
At this point, we know if the Entry is pure text or YAML so that we can convert the content as text properly.
Did I miss something? 🤔

Oh, I'll check your comment and see it again. Thank you!
I saw the assertion fails with JSON format content at https://github.com/line/centraldogma/pull/616/files#diff-32bfb9d1fc9f4d4388019340cc31985f5ac3e41dd32aa11283cd78805398ba57R74-R76, so I thought as I wrote. Maybe the code I wrote is doing something wrong.
(I was working on checking 'EntryType', but something was wrong, so I reverted it. I can't remember what it was, but I'll see it again!)
I'll have to check again after work! 😄

minwoox · 2021-07-28T07:26:32Z

I saw the assertion fails with JSON format content at #616 (files), so I thought as I wrote. Maybe the code I wrote is doing something wrong.

That's probably because we didn't modify the client? 😄
https://github.com/line/centraldogma/blob/master/client/java-armeria/src/main/java/com/linecorp/centraldogma/client/armeria/ArmeriaCentralDogma.java#L463-L464

ikhoon · 2021-07-29T14:24:24Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

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

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

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

common/src/main/java/com/linecorp/centraldogma/internal/Util.java

ikhoon · 2021-07-29T14:42:48Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

@@ -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;


If patch is not used outside of case, isn't it better to revert and minimize its scope?

I'll revert it when when making Jackson.readTree() receive EntryType.

ikhoon · 2021-07-29T14:52:56Z

...ain/java/com/linecorp/centraldogma/server/internal/storage/repository/git/GitRepository.java

+                                final JsonNode oldYamlNode =
+                                        JacksonYaml.readTree(
+                                                reader.open(diffEntry.getOldId().toObjectId()).getBytes());
+                                final JsonNode newYamlNode =
+                                        JacksonYaml.readTree(
+                                                reader.open(diffEntry.getNewId().toObjectId()).getBytes());


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 } } ... }

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(...)

chacham · 2021-08-01T09:11:44Z

Because the Jackson is used for both Entry content and metadata/request/response, some methods are duplicated and confusing.
The methods for the content of Entry has been modified to receive EntryType as a parameter, but there are still methods without EntryType(for metadata/request/response), so it looks easy to make mistake. (maybe I did already)
Would it better to separate Jackson for Content and Jackson for Metadata? Could somebody help me? 🤔

minwoox · 2021-08-02T02:00:24Z

but there are still methods without EntryType(for metadata/request/response), so it looks easy to make mistake. (maybe I did already)

Could you give us a specific example of the case, please?

chacham · 2021-08-02T11:59:53Z

Could you give us a specific example of the case, please?

Jackson.readTree() for parsing git revision from full commit messgae
- https://github.com/line/centraldogma/blob/master/server/src/main/java/com/linecorp/centraldogma/server/internal/storage/repository/git/CommitUtil.java#L60
Jackson.writeValueAsBytes() for write session for file based management
- https://github.com/line/centraldogma/blob/master/server/src/main/java/com/linecorp/centraldogma/server/internal/admin/auth/FileBasedSessionManager.java#L185
Jackson.writeValueAsPrettyString() for data objects toString() (not related to Entry)
- AuthConfig, CentralDogmaConfig, ClientProfile, SamlAuthProperties

Especially, Jackson.writeValueAsBytes() is used frequently for HTTP/File/ZooKeeper (where entries are not related)
There are two implementations, one for with EntryType, and one for without EntryType

ikhoon · 2021-08-02T13:35:37Z

Would it better to separate Jackson for Content and Jackson for Metadata?

Jackson should be not strongly connected to any domain. As you know it is a thin wrapper of ObjectMapper.
What do you think about #616 (comment) style?
You don't want to implement some methods that are not used in YAML, we can introduce an abstract class.
Additionally, some methods using compactFactory, prettyFactory could be moved to JacksonUtil?

An interface for factory methods and APIs.

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);
   ...
}

An abstract class for marshaling and unmarshaling objects

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
    ...
}

Create JacksonJson and JacksonYaml

// 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);
    }
}

ikhoon · 2021-08-02T13:39:05Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

Any though on this? @minwoox

chacham · 2021-08-02T13:45:21Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

Any though on this? @minwoox

It seems there are several @ParameterizedTest. Should the tests be separated if the new features do not support legacy client?

ikhoon · 2021-08-02T13:50:28Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

Any though on this? @minwoox

It seems there are several @ParameterizedTest. Should the tests be separated if the new features do not support legacy client?

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.

chacham · 2021-08-02T14:06:00Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

Any though on this? @minwoox

It seems there are several @ParameterizedTest. Should the tests be separated if the new features do not support legacy client?

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'll ask for help if there's any difficulties. 😁

minwoox · 2021-08-02T14:16:42Z

Do we need to add new features for the legacy Thrift client and service? 🤔
How about only supporting ArmeriaCentralDogma and REST API?

👍

chacham · 2021-08-08T10:16:28Z

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.
Now, there are many Jackson.ofJson(). I'm confused if I'm going in the right direction. 😵‍💫

minwoox · 2021-08-10T08:57:16Z

Now, there are many Jackson.ofJson(). I'm confused if I'm going in the right direction.

I think it's okay! Please, go ahead! 😄

minwoox · 2021-11-25T08:12:52Z

I guess this is ready so let me change the status of this PR. 😉

minwoox · 2021-11-25T08:29:03Z

Let me do the final review after #616 (comment) is addressed. 🙇‍♂️

chacham · 2021-11-25T16:26:09Z

Old clients crashes when EntryType.YAML goes down to the client.
I tested dogma.listFiles() after making yaml file, and below is the log.

java.lang.IllegalArgumentException: No enum constant com.linecorp.centraldogma.common.EntryType.YAML
java.util.concurrent.ExecutionException: java.lang.IllegalArgumentException: No enum constant com.linecorp.centraldogma.common.EntryType.YAML
	at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:395)
	at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1999)
	at com.example.demo.CentralDogmaClientTest.listFiles(CentralDogmaClientTest.java:37)

minwoox · 2021-11-26T15:37:29Z

Old clients crashes when EntryType.YAML goes down to the client.
I tested dogma.listFiles() after making yaml file, and below is the log.

Had a chat with @ikhoon and I think it's time to go for /api/v2/ https://github.com/line/centraldogma/blob/master/common/src/main/java/com/linecorp/centraldogma/internal/api/v1/HttpApiV1Constants.java#L27
which will support YAML.
Also, we can add JSON5 support with /api/v2/. /cc @ks-yim

I think there's a lot to do to use /api/v2 so let's finish this PR by just implementing YAML.
I might implement /api/v2/ based on your work. 😄

Supports YAML Entry and upsert YAML

minwoox added the new feature label Jul 7, 2021

minwoox reviewed Jul 7, 2021

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

minwoox reviewed Jul 12, 2021

View reviewed changes

chacham force-pushed the feature/yaml-support branch from a15084a to 6299f07 Compare July 18, 2021 06:58

ikhoon reviewed Jul 29, 2021

View reviewed changes

chacham force-pushed the feature/yaml-support branch 2 times, most recently from f1bb7e9 to 1960c21 Compare August 2, 2021 13:32

minwoox marked this pull request as ready for review November 25, 2021 08:12

minwoox requested review from jrhee17 and trustin as code owners November 25, 2021 08:12

chacham force-pushed the feature/yaml-support branch from 33ae5ac to f2332e2 Compare November 25, 2021 15:56

chacham added 20 commits January 6, 2022 20:55

Add basic YAML support to GitRepository

340d019

Supports YAML Entry and upsert YAML

Add basic client supports for YAML

5257fdb

Change direct use of SnakeYAML to jackson-dataformat-yaml

3dd4cb5

Make YAML patch using JsonPatch

ff399d9

Check EntryType in ContentHolder for contentAsText

33b8720

Refactor JSON/YAML file path related namings

8ce989a

Refactor Jackson to use EntryType

43de1ac

Implement YAML patch for legacy client

987f89f

Merge JacksonJson/JacksonYaml into Jackson

005b008

Merge duplicated JSON/YAML code in GitRepository.toChangeMap()

7d4180e

Extract entry type validation for LegacyCentralDogma.toEntry()

6c0adfe

Handle YAML EntryType when git mirror remote to local

934c0d2

Fix APPLY_YAML_PATCH for legacy client

e69c9fc

Refactor Jackson to interface and make use JSON/YAML implementations

49f2298

Fail on yaml duplicate key to block silent overwrite

fae77f2

Use HttpResponse.ofJson when possible

4cf33c3

Change Jackson interface to abstract class

4be6510

Rename confusing file name validation method

f44a022

Remove unused prettyMapper from Jackson

93e1136

Add MergeQuery.ofYaml

8c2ebb1

chacham force-pushed the feature/yaml-support branch from f2332e2 to 8c2ebb1 Compare January 6, 2022 12:39

Dogacel mentioned this pull request Mar 1, 2024

Better YAML support #151

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add better YAML supports #616

Add better YAML supports #616

chacham commented Jul 6, 2021 •

edited

CLAassistant commented Jul 6, 2021 •

edited

codecov bot commented Jul 6, 2021 •

edited

minwoox commented Jul 7, 2021 •

edited

minwoox left a comment

minwoox Jul 12, 2021

chacham commented Jul 18, 2021

minwoox commented Jul 19, 2021

chacham commented Jul 27, 2021 •

edited

minwoox commented Jul 28, 2021 •

edited

chacham commented Jul 28, 2021

minwoox commented Jul 28, 2021 •

edited

chacham commented Jul 28, 2021 •

edited

minwoox commented Jul 28, 2021

ikhoon commented Jul 29, 2021 •

edited

ikhoon Jul 29, 2021

chacham Jul 31, 2021

ikhoon Jul 29, 2021 •

edited

ikhoon Jul 29, 2021

chacham commented Aug 1, 2021

minwoox commented Aug 2, 2021

chacham commented Aug 2, 2021

ikhoon commented Aug 2, 2021 •

edited

ikhoon commented Aug 2, 2021

chacham commented Aug 2, 2021 •

edited

ikhoon commented Aug 2, 2021

chacham commented Aug 2, 2021

minwoox commented Aug 2, 2021

chacham commented Aug 8, 2021

minwoox commented Aug 10, 2021

minwoox commented Nov 25, 2021 •

edited

minwoox commented Nov 25, 2021

chacham commented Nov 25, 2021

minwoox commented Nov 26, 2021 •

edited

Add better YAML supports #616

Are you sure you want to change the base?

Add better YAML supports #616

Conversation

chacham commented Jul 6, 2021 • edited

Motivation

Modifications

In Progress

TO-DO (Maybe later)

CLAassistant commented Jul 6, 2021 • edited

codecov bot commented Jul 6, 2021 • edited

Codecov Report

minwoox commented Jul 7, 2021 • edited

minwoox left a comment

Choose a reason for hiding this comment

minwoox Jul 12, 2021

Choose a reason for hiding this comment

chacham commented Jul 18, 2021

minwoox commented Jul 19, 2021

chacham commented Jul 27, 2021 • edited

minwoox commented Jul 28, 2021 • edited

chacham commented Jul 28, 2021

minwoox commented Jul 28, 2021 • edited

chacham commented Jul 28, 2021 • edited

minwoox commented Jul 28, 2021

ikhoon commented Jul 29, 2021 • edited

ikhoon Jul 29, 2021

Choose a reason for hiding this comment

chacham Jul 31, 2021

Choose a reason for hiding this comment

ikhoon Jul 29, 2021 • edited

Choose a reason for hiding this comment

ikhoon Jul 29, 2021

Choose a reason for hiding this comment

chacham commented Aug 1, 2021

minwoox commented Aug 2, 2021

chacham commented Aug 2, 2021

ikhoon commented Aug 2, 2021 • edited

ikhoon commented Aug 2, 2021

chacham commented Aug 2, 2021 • edited

ikhoon commented Aug 2, 2021

chacham commented Aug 2, 2021

minwoox commented Aug 2, 2021

chacham commented Aug 8, 2021

minwoox commented Aug 10, 2021

minwoox commented Nov 25, 2021 • edited

minwoox commented Nov 25, 2021

chacham commented Nov 25, 2021

minwoox commented Nov 26, 2021 • edited

chacham commented Jul 6, 2021 •

edited

CLAassistant commented Jul 6, 2021 •

edited

codecov bot commented Jul 6, 2021 •

edited

minwoox commented Jul 7, 2021 •

edited

chacham commented Jul 27, 2021 •

edited

minwoox commented Jul 28, 2021 •

edited

minwoox commented Jul 28, 2021 •

edited

chacham commented Jul 28, 2021 •

edited

ikhoon commented Jul 29, 2021 •

edited

ikhoon Jul 29, 2021 •

edited

ikhoon commented Aug 2, 2021 •

edited

chacham commented Aug 2, 2021 •

edited

minwoox commented Nov 25, 2021 •

edited

minwoox commented Nov 26, 2021 •

edited