Add JSON5 support

[ks-yim]

Motivation:

• #538

Modifications:

• Server & ArmeriaCentralDogma client JSON5 support

  • getFile() as Entry<JsonNode> but can still get the raw content via #contentAsText()
  • watchFile() as Entry<JsonNode> but can still get the raw content via #contentAsText()
  • getFile() & watchFile() with JSON_PATH
  • mergeFiles()

• Update Admin UI's ace editor version to 1.4.13

  • To support JSON5 syntax highlighting

Results:

• Closes #538

[CLAassistant]

All committers have signed the CLA.

[minwoox]

This looks awesome. 😆

[codecov[bot]]

Codecov Report

Patch coverage: 94.39% and project coverage change: +0.29 :tada:

Comparison is base (0ed7a9a) 70.04% compared to head (849bdb2) 70.34%.

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     

Impacted Files	Coverage Δ
...ntraldogma/client/armeria/ArmeriaCentralDogma.java	72.89% <80.00%> (-0.02%)	:arrow_down:
...java/com/linecorp/centraldogma/internal/Json5.java	88.88% <88.88%> (ø)
.../java/com/linecorp/centraldogma/common/Change.java	76.40% <91.66%> (+1.11%)	:arrow_up:
...n/java/com/linecorp/centraldogma/common/Entry.java	93.33% <92.85%> (-0.55%)	:arrow_down:
...om/linecorp/centraldogma/common/DefaultChange.java	74.62% <100.00%> (+1.18%)	:arrow_up:
...va/com/linecorp/centraldogma/common/EntryType.java	75.00% <100.00%> (ø)
.../java/com/linecorp/centraldogma/internal/Util.java	70.64% <100.00%> (+0.90%)	:arrow_up:
...inecorp/centraldogma/internal/api/v1/EntryDto.java	70.58% <100.00%> (ø)
...p/centraldogma/internal/api/v1/WatchResultDto.java	54.54% <100.00%> (ø)
...raldogma/server/internal/api/ContentServiceV1.java	85.92% <100.00%> (+0.10%)	:arrow_up:
... and 4 more

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[ks-yim]

@jrhee17 @ikhoon @minwoox I've re-written the PR as per your advices, PTAL 🙏.

• Use EntryType.JSON for JSON5
• Use dedicated mapper for JSON5
• Give higher priority to contentAsText property when transmitting JSON5 content of Entry or Change over the wire.

Sorry that it's delayed. 😓

[ks-yim]

@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 ALLOW_SINGLE_QUOTES features when it deserializes JSON5 from a byte-array source.

I've summarized the issue at Json5#readTree(byte[]) method...:

// 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 byte[] into String before deserializing JSON5 content as of now.

[minwoox]

And I added a validation before committing a patch on JSON5 files (since they are updated via text patch..).

👍

I will report the issue to Jackson but I think we should always convert byte[] into String before deserializing JSON5 content as of now.

Oops, I didn't notice that. 😅 Yeah, let's report it to the upstream and use String for now.

except the feature of not notifying watchers for any changes that does not affect the actual content of JSON5.

Could you tell me more about it, please?

[minwoox]

I think it's almost done, excellent job, @ks-yim! https://github.com/line/centraldogma/runs/5271252784?check_suite_focus=true#step:5:1211 Could you check the test failures, please? 😄

[ks-yim]

@minwoox Thanks for the patience.

It seems Jackson's ALLOW_BACKSLASH_ESCAPING_ANY_CHARACTER feature doesn't work well in Windows..? I should test it in my Windows laptop.

[minwoox]

@minwoox Thanks for the patience.

I really appreciate all the efforts. Please, don't worry about it. 😄

It seems Jackson's ALLOW_BACKSLASH_ESCAPING_ANY_CHARACTER feature doesn't work well in Windows..? I should test it in my Windows laptop.

Oops, let me also take a look at it.

[jyterencekim]

👀 @ks-yim Thanks for your work. I was just wondering whether this is still in progress.

[ks-yim]

@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 FileType, a more structured approach is needed so that other config formats(.yaml, .toml, etc.) support can be added without pain later in the future, too.

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 EntryType.TEXT when it is patched & stored but should support ALL EntryType.JSON features such as..:

• getFile() as Entry<JsonNode> but can still get the raw content via #contentAsText()
• watchFile() as Entry<JsonNode> but can still get the raw content via #contentAsText()
• getFile() & watchFile() with JSON_PATH
• mergeFiles()

[minwoox]

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

[ks-yim]

@minwoox thanks you for all the valuable feedback you gave while I am working on this 😄.

Closing this PR in favor of API v2.

[minwoox]

What I meant was I was going to work on this PR again after adding V2. 😓 Could you reopen this?

[ks-yim]

@minwoox Oops, my mistake 🤣. Reopened the PR! Looking forward to the rework!