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.

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

Introduce object creation interface with initial values #756

Merged
merged 36 commits into from
Jan 17, 2024

Conversation

highcloud100
Copy link
Contributor

@highcloud100 highcloud100 commented Jan 9, 2024

What this PR does / why we need it:

There was no way to set nested object in one set operation.
So It needed multiple set operation.

To resolve this issues, this pr modify the set operation to create a nested object in one operation.

More details refer the follow link.
yorkie-team/yorkie-js-sdk#691 (comment)

Which issue(s) this PR fixes:

Fixes #663

Does this PR introduce a user-facing change?:

  1. Now users can set object with json in one operation.

As-is

  • multiple set operation
err = d1.Update(func(root *json.Object, p *presence.Presence) error {
	  root.SetNewObject("obj").SetString("str", "v")
	  root.GetObject("obj").SetNewArray("arr").AddInteger(1).AddString("str")
	  root.GetObject("obj").SetNewObject("obj").SetDouble("key3", 42.2)
	  root.GetObject("obj").SetNewCounter("cnt", crdt.LongCnt, 0)
	  root.GetObject("obj").SetNewText("txt")
	  root.GetObject("obj").SetNewTree("tree", &json.TreeNode{
		  Type: "doc",
		  Children: []json.TreeNode{{
			  Type: "p", Children: []json.TreeNode{{Type: "text", Value: "ab"}},
		  }},
	  })
	  return nil
})

To-be

  • single set operation
err := d1.Update(func(root *json.Object, p *presence.Presence) error {
	root.SetNewObject("obj", map[string]interface{}{ // 1: object
		"str": "v",                                  // 1: string
		"arr": []interface{}{1, "str"},              // 3: array, 1, "str"
		"obj": map[string]interface{}{"key3": 42.2}, // 2: object, 42.2
		"cnt": json.NewCounter(0, crdt.LongCnt),     // 1: counter
		"txt": json.NewText(),                       // 1: text
		"tree": json.NewTree(&json.TreeNode{   // 1: tree
			Type: "doc",
			Children: []json.TreeNode{{
				Type: "p", Children: []json.TreeNode{{Type: "text", Value: "ab"}},
			}},
		}),
	})
	return nil
})
  1. Now user can set array with value in one operation.

As-is

root.SetNewArray("k1").AddString("1", "2", "3")

To be

root.SetNewArray("k1", []interface{}{"1", "2", "3"})

Limitations

  1. Current users can only set the array initial value to []interface{} without any specific type, this problem also occurs when set an object with the json literal.
root.SetNewArray("k1", []interface{}{"1", "2", "3"})
root.SetNewArray("k1", []int{"1", "2", "3"}) // does not support now

refer the implementation of "encoding/json"

  1. There is no way to set Text with initial value.
err := d1.Update(func(root *json.Object, p *presence.Presence) error {
	root.SetNewObject("obj", map[string]interface{}{
                "txt": json.NewText(), 
		"txt": json.NewText("initial text"),  // does not support now                  
	})
	return nil
})

Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@highcloud100 highcloud100 marked this pull request as draft January 9, 2024 04:59
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (145d4f8) 49.35% compared to head (cee9b6d) 50.14%.
Report is 1 commits behind head on main.

Files Patch % Lines
pkg/document/crdt/array.go 20.00% 3 Missing and 1 partial ⚠️
pkg/document/crdt/object.go 20.00% 3 Missing and 1 partial ⚠️
pkg/document/crdt/root.go 86.66% 3 Missing and 1 partial ⚠️
pkg/document/crdt/counter.go 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #756      +/-   ##
==========================================
+ Coverage   49.35%   50.14%   +0.79%     
==========================================
  Files          69       69              
  Lines       10113    10182      +69     
==========================================
+ Hits         4991     5106     +115     
+ Misses       4606     4534      -72     
- Partials      516      542      +26     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@highcloud100 highcloud100 marked this pull request as ready for review January 9, 2024 05:21
@highcloud100 highcloud100 marked this pull request as draft January 9, 2024 07:40
@highcloud100 highcloud100 marked this pull request as ready for review January 9, 2024 07:47
Copy link
Contributor

@chacha912 chacha912 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!
I've left some comments, please check them.

pkg/document/json/json.go Show resolved Hide resolved
pkg/document/json/json.go Outdated Show resolved Hide resolved
pkg/document/json/json.go Outdated Show resolved Hide resolved
pkg/document/json/object.go Outdated Show resolved Hide resolved
test/integration/object_test.go Outdated Show resolved Hide resolved
pkg/document/crdt/root.go Show resolved Hide resolved
pkg/document/crdt/root.go Outdated Show resolved Hide resolved
test/integration/object_test.go Outdated Show resolved Hide resolved
@highcloud100 highcloud100 marked this pull request as draft January 10, 2024 05:42
@highcloud100 highcloud100 marked this pull request as ready for review January 12, 2024 04:10
Add buildArrayMember
Refactor NewArray, NewObject, buildCRDTElement, counter type test code
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@hackerwins hackerwins merged commit 65712ec into main Jan 17, 2024
3 checks passed
@hackerwins hackerwins deleted the set-nested-object branch January 17, 2024 11:23
@hackerwins hackerwins changed the title Enhance Set operation for representing nested json literal Introduce object creation interface with initial values Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the set operation to handle nested object values
3 participants