Remove panic from crdt primitive #636

fourjae · 2023-09-05T16:07:45Z

What this PR does / why we need it:

Action to remove panic method from crdt/primitive.go

I am leaving a question because I encountered a problem while uninstalling crdt/primitive.go Panic.

In the case of the func (p *Primitive) Marshal() method inside the crdt/primitive.go package, Marshal() of the type Element interface is implemented and used.

However, if you modify the interface to Marshal() (string, error), the related implementations must be modified.

At this time, Marshal() is called when calling other String() methods, and I am asking because I think it will be difficult to change this part of String().

yorkie/pkg/document/crdt/rga_tree_list.go

Line 89 in 1b557be

func (n *RGATreeListNode) String() string {

Please let me know if I'm missing something or if there's a better way. thank you

Which issue(s) this PR fixes:

Fixes #497 crdt/primitive

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Additional documentation:

Checklist:

Added relevant tests or not required
Didn't break anything

krapie · 2023-09-06T10:25:41Z

@fourjae I think we need to change Marshal() string interface to Marshal() (string, error), just like when we removed panic() in crdt/text.

In #522, we changed DeepCopy() Element signature to DeepCopy() (Element, error), and updated all related codes.

Changing Marshal() will affect many other related codes including String(), but I think we need to update them all. It will take some time.

fourjae · 2023-09-06T15:54:29Z

As we progressed, we encountered this problem. What do you think?

I'm trying to fix crdt/primitive marshal(), but I have the following problem.

Marshal() implements Marshal() of type Element interface. Therefore, the return format has been modified to (String, error) and is in progress.
Among the implemented Marshal()s, Marshal(), which receives the *Object structure as an argument, is used when calling from String() in crdt/rga_tree_list.go.
The String() is used in pkg/llrb.go and pkg/splay.go and implements the type Value interface. String() is implemented and used in approximately 12,000 places.

As a result, all String() methods must be changed to handle errors in marshal().

Do you have any good troubleshooting methods?

krapie · 2023-09-12T03:30:35Z

As we progressed, we encountered this problem. What do you think?

I'm trying to fix crdt/primitive marshal(), but I have the following problem.

Marshal() implements Marshal() of type Element interface. Therefore, the return format has been modified to (String, error) and is in progress.

Among the implemented Marshal()s, Marshal(), which receives the *Object structure as an argument, is used when calling from String() in crdt/rga_tree_list.go.

The String() is used in pkg/llrb.go and pkg/splay.go and implements the type Value interface. String() is implemented and used in approximately 12,000 places.

As a result, all String() methods must be changed to handle errors in marshal().

Do you have any good troubleshooting methods?

@hackerwins Any ideas on this issue?

krapie

For discussion record: It will be nice to move panic() to the json layer so that we can catch errors at the json level and change types in the crdt layer to never emit errors on the crdt layer. This will prevent interface changes on crdt layer.

codecov · 2023-09-20T13:29:30Z

Codecov Report

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

Comparison is base (940941a) 49.47% compared to head (7aad26d) 49.31%.
Report is 1 commits behind head on main.

❗ Current head 7aad26d differs from pull request most recent head b5f4740. Consider uploading reports for the commit b5f4740 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #636      +/-   ##
==========================================
- Coverage   49.47%   49.31%   -0.16%     
==========================================
  Files          69       69              
  Lines        9951     9973      +22     
==========================================
- Hits         4923     4918       -5     
- Misses       4512     4532      +20     
- Partials      516      523       +7

Files	Coverage Δ
api/converter/to_bytes.go	`57.37% <ø> (ø)`
api/converter/to_pb.go	`52.75% <ø> (ø)`
pkg/document/crdt/rga_tree_list.go	`76.68% <50.00%> (-1.22%)`	⬇️
pkg/document/crdt/array.go	`32.25% <0.00%> (-0.71%)`	⬇️
api/converter/from_bytes.go	`43.47% <25.00%> (-1.88%)`	⬇️
api/converter/from_pb.go	`41.28% <35.71%> (-0.81%)`	⬇️
pkg/document/crdt/primitive.go	`91.57% <70.96%> (-2.11%)`	⬇️

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

fourjae · 2023-09-20T13:48:44Z

There were things I didn't know about, and after analyzing them, I came to the following conclusion and changed it to PR.

(p *Primitive) As a result of checking the primitive type of the Marshal() function, there are 8 types (Null, Boolean, Integer, Long, Double, String, Bytes, Date) already declared as constants. Only the above 8 types could be executed.
Therefore, the panic itself could not occur.

yorkie/pkg/document/crdt/primitive.go

Line 31 in 940941a

// Primitive can have the following types:
In the NewPrimitive function, the exception type (float) is also set to Double and values other than those declared in the constant cannot be entered.
(p *Primitive) Marshal() function is Since we are not calling from json layer, we don't need to use any method to catch error at json level.

Therefore, the (p *Primitive) Marshal() function only erased the panic from the crdt layer.

krapie

Thank you for your contribution.
I left some comments below.

pkg/document/crdt/primitive.go

pkg/document/crdt/object.go

pkg/document/crdt/element_rht.go

pkg/document/crdt/array.go

pkg/document/json/counter.go

pkg/splay/splay.go

test/integration/array_test.go

pkg/document/json/object.go

Changing mismatched code conventions and changing err handling

fourjae · 2023-09-21T14:51:49Z

Thanks for your comment!

Fixed code conventions that did not fit.

The hard-coded err message has been changed to return an err variable.

Please let me know if you need any additional changes.

krapie

Thank you for reflecting my review.
I left just few more comments 😄

krapie · 2023-09-28T13:20:34Z

pkg/document/json/array.go

@@ -194,7 +227,6 @@ func (p *Array) addInternal(
 ) crdt.Element {
 	return p.insertAfterInternal(p.Array.LastCreatedAt(), creator)
 }
-


It will be nice to have space between functions

krapie · 2023-09-28T13:25:50Z

pkg/document/json/object.go

@@ -120,34 +124,47 @@ func (p *Object) SetNewTree(k string, initialRoot ...*TreeNode) *Tree {
 // SetNull sets the null for the given key.
 func (p *Object) SetNull(k string) *Object {
 	p.setInternal(k, func(ticket *time.Ticket) crdt.Element {
-		return crdt.NewPrimitive(nil, ticket)


It will be nice to keep space convention of spacing before return in this file. I will leave only one comment for this, so check all the return codes in this file.

krapie · 2023-09-28T13:29:04Z

pkg/document/crdt/primitive.go

 }

 // Marshal returns the JSON encoding of the value.
 func (p *Primitive) Marshal() string {
 	switch p.valueType {
 	case Null:
-		return "null"
+		return ""


Is there any reason for this change? I will be better to keep the previous code.

krapie · 2023-09-28T13:29:52Z

pkg/document/crdt/primitive_test.go

@@ -34,7 +34,7 @@ func TestPrimitive(t *testing.T) {
 		valueType crdt.ValueType
 		marshal   string
 	}{
-		{nil, crdt.Null, "null"},
+		{nil, crdt.Null, ""},


Is there any reason for this change? I will be better to keep the previous code.

It will be better to revert this code too.

krapie · 2023-09-28T13:33:08Z

pkg/document/crdt/primitive.go

@@ -213,9 +213,10 @@ func (p *Primitive) Marshal() string {
 		return fmt.Sprintf(`"%s"`, p.value)
 	case Date:
 		return fmt.Sprintf(`"%s"`, p.value.(gotime.Time).Format(gotime.RFC3339))
+		// when Null or default


It will be nice to remove comment.

…rollback to String "null"

krapie

Looks good to me.

Could you review this PR @hackerwins?

krapie · 2023-09-28T16:56:03Z

pkg/document/json/array.go

 		})
 	}
-


Also check all the return codes for spacing convention in this file.

hackerwins

Thanks for your contribution. 👍

krapie requested review from hackerwins and krapie September 5, 2023 16:29

krapie assigned fourjae Sep 5, 2023

krapie added the cleanup 🧹 Paying off technical debt label Sep 5, 2023

krapie removed their request for review September 5, 2023 16:33

krapie self-requested a review September 6, 2023 10:28

krapie reviewed Sep 16, 2023

View reviewed changes

fourjae added 4 commits September 20, 2023 22:00

crdt/primitive task progress

3d71c56

except Marshall() Remove panic method from crdt/primitive

2992e56

Convert from Marshal() of type (string, error) to type string

a792351

Added checking logic for unused err

bb79fcd

fourjae force-pushed the remove-panic-from-crdt-primitive branch from 5b78e7d to bb79fcd Compare September 20, 2023 13:15

Delete the commented crdt/primitive panic function

0e4b28f

fourjae marked this pull request as ready for review September 20, 2023 13:50

Remove duplicate test code in "increase test"

5d387ae

krapie self-requested a review September 21, 2023 03:19

krapie requested changes Sep 21, 2023

View reviewed changes

Reflecting krapie's comment

7aad26d

Changing mismatched code conventions and changing err handling

fourjae requested a review from krapie September 26, 2023 13:15

krapie requested changes Sep 28, 2023

View reviewed changes

Reflecting krapie's comment Changing mismatched code conventions and …

b5f4740

…rollback to String "null"

fourjae requested a review from krapie September 28, 2023 15:29

krapie reviewed Sep 28, 2023

View reviewed changes

pkg/document/json/array.go

})

}

Copy link

Member

krapie Sep 28, 2023

Choose a reason for hiding this comment

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

Also check all the return codes for spacing convention in this file.

hackerwins approved these changes Sep 29, 2023

View reviewed changes

hackerwins merged commit 34c8505 into yorkie-team:main Sep 29, 2023
1 check passed

hackerwins pushed a commit that referenced this pull request Sep 29, 2023

Remove panic from crdt.Primitive (#636)

b241880

hackerwins pushed a commit that referenced this pull request Oct 1, 2023

Remove panic from crdt.Primitive (#636)

b6889c9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove panic from crdt primitive #636

Remove panic from crdt primitive #636

fourjae commented Sep 5, 2023 •

edited

Loading

krapie commented Sep 6, 2023 •

edited

Loading

fourjae commented Sep 6, 2023 •

edited by krapie

Loading

krapie commented Sep 12, 2023

krapie left a comment

codecov bot commented Sep 20, 2023 •

edited

Loading

fourjae commented Sep 20, 2023

krapie left a comment

fourjae commented Sep 21, 2023

krapie left a comment

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie Sep 28, 2023

krapie left a comment

krapie Sep 28, 2023

hackerwins left a comment

Remove panic from crdt primitive #636

Remove panic from crdt primitive #636

Conversation

fourjae commented Sep 5, 2023 • edited Loading

Action to remove panic method from crdt/primitive.go

krapie commented Sep 6, 2023 • edited Loading

fourjae commented Sep 6, 2023 • edited by krapie Loading

krapie commented Sep 12, 2023

krapie left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 20, 2023 • edited Loading

Codecov Report

fourjae commented Sep 20, 2023

krapie left a comment

Choose a reason for hiding this comment

fourjae commented Sep 21, 2023

krapie left a comment

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

krapie left a comment

Choose a reason for hiding this comment

krapie Sep 28, 2023

Choose a reason for hiding this comment

hackerwins left a comment

Choose a reason for hiding this comment

fourjae commented Sep 5, 2023 •

edited

Loading

krapie commented Sep 6, 2023 •

edited

Loading

fourjae commented Sep 6, 2023 •

edited by krapie

Loading

codecov bot commented Sep 20, 2023 •

edited

Loading