add: MarshalJSON methods for Array and Document #70

KrishnaSindhur · 2024-12-18T09:49:57Z

Closes #49

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

wirebson/document.go

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

Copilot

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

wirebson/document_test.go:54

Consider adding a test case for marshalling an empty Document to ensure it marshals correctly.

t.Run("MarshalJSON_PreserveOrder", func(t *testing.T) {

wirebson/document.go:261

The error message 'nil Document' is unclear. It should be more descriptive, such as 'Document is nil and cannot be marshaled to JSON'.

return nil, lazyerrors.Errorf("nil Document")

wirebson/document.go:259

The manual construction of the JSON object in 'Document.MarshalJSON' is error-prone. Consider using 'json.Marshal' for the entire 'doc.fields' slice to simplify the implementation and avoid potential issues.

func (doc *Document) MarshalJSON() ([]byte, error) {

wirebson/array.go:185

[nitpick] The error message 'nil Array' is unclear. Consider changing it to 'Cannot marshal a nil Array'.

return nil, lazyerrors.Errorf("nil Array")

codecov · 2024-12-19T05:15:54Z

Codecov Report

Attention: Patch coverage is 61.29032% with 12 lines in your changes missing coverage. Please review.

Project coverage is 62.96%. Comparing base (725f032) to head (cbfdd87).

Files with missing lines	Patch %	Lines
wirebson/document.go	65.38%	6 Missing and 3 partials ⚠️
wirebson/array.go	40.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
- Coverage   63.03%   62.96%   -0.07%     
==========================================
  Files          41       41              
  Lines        2221     2252      +31     
==========================================
+ Hits         1400     1418      +18     
- Misses        645      654       +9     
- Partials      176      180       +4

Files with missing lines	Coverage Δ
wirebson/array.go	`61.70% <40.00%> (-1.22%)`	⬇️
wirebson/document.go	`48.07% <65.38%> (+3.46%)`	⬆️

... and 1 file with indirect coverage changes

Flag	Coverage Δ
test	`62.96% <61.29%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

AlekSi

Please also run linters locally

wirebson/array.go

AlekSi · 2024-12-19T06:16:53Z

wirebson/array_test.go

Please update tests, benchmarks, and fuzzing in bson_test.go instead

Please don't resolve conversations. Please also update existing tests/benchmarks/fuzzes, not add new ones.

AlekSi · 2024-12-19T06:18:20Z

wirebson/document.go

+	jsonObject := make([]byte, 0)
+	jsonObject = append(jsonObject, '{')


Suggested change

jsonObject := make([]byte, 0)

jsonObject = append(jsonObject, '{')

res := []byte{'{'}

@AlekSi any changes required here? I have updated to must.NotBeZero(doc)

Well, yeah, I even wrote them

I'm not sure why this conversation was resolved without changes applied

I have not added test for error scenarios In MarshalJSON is it ok? Because I don't see any error scenarios tests added for encode functions also!! Please correct me if I am wrong. If necessary I can add test for error scenarios.

This comment does not belong to this conversation, where I asked you to replace make+append combo with a single assignment, and even provided a suggested change

Sorry but I can't see your comments you added to replace make+append to single line, Which ended with some confusions.

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

AlekSi

Review comments were "resolved", but not addressed

AlekSi · 2024-12-19T09:08:18Z

wirebson/bson_test.go

 				assert.Equal(t, tc.raw, raw)
 			})
+
+			t.Run("ArrayMarshalJSON", func(t *testing.T) {


There is no point in running those subtests for each tc with the same input and expected output. Instead, all subtests use tc.

Please slow down, check all tests in this file, and make sure your changes follow its structure.

AlekSi · 2024-12-20T07:51:07Z

wirebson/bson_test.go

 	},
 }

+var marshalJSONTestCases = []normalTestCase{


Please update existing normalTestCases variable instead

AlekSi

The main comment: please follow the existing code (and tests) structure

AlekSi · 2024-12-23T09:23:46Z

wirebson/array_test.go

+		data, err := json.Marshal(array)
+		assert.NoError(t, err)
+		expected := `["value1",42,true]`
+		assert.JSONEq(t, expected, string(data))


assert.JSONEq, per its documentation, ignores the filed order – something we care about.

I'm also not sure why we have that test at all, given that bson_test.go should already test it.

AlekSi · 2024-12-23T09:25:15Z

wirebson/bson_test.go

+	raw           RawDocument
+	doc           *Document
+	mi            string
+	isMarshalJSON bool // isMarshalJSON: Set to true if the test case is meant for JSON marshalling.


All tests should be meant for JSON marshaling and unmarshalling.

Check how mi, doc, and raw are used. Add another field that would contain expected JSON representation and test both decoding and encoding.

AlekSi · 2024-12-23T09:26:33Z

wirebson/bson_test.go

+	{
+		name: "MarshalJsonOrderedFields",
+		doc: MustDocument(
+			"first", int32(1),
+			"second", "two",
+			"third", float64(3.0),
+		),
+		mi: `{
+			"first": 1,
+			"second": "two",
+			"third": 3.0
+		}`,
+		isMarshalJSON: true,
+	},
+	{
+		name:          "MarshalJsonEmptyDocument",
+		doc:           MustDocument(),
+		mi:            `{}`,
+		isMarshalJSON: true,
+	},


Please update existing test cases (I said that before, didn't I?), not add new ones. Please do not try different ways of adding only a few tests instead of updating existing ones.

AlekSi · 2024-12-23T09:26:59Z

wirebson/bson_test.go

-				ls := doc.LogValue().Resolve().String()
-				assert.NotContains(t, ls, "panicked")
-				assert.NotContains(t, ls, "called too many times")
+			if tc.raw != nil {


Please revert that change

AlekSi · 2024-12-23T09:27:35Z

wirebson/document.go

+// MarshalJSON implements the json.Marshaler interface for Document.
+// It converts the Document into a JSON object representation while preserving the order of fields.


Note the format of comments for methods that implement the interface and do the same.

AlekSi · 2024-12-25T12:24:49Z

I made nessisary changes in #71. Thank you!

add: MarshalJSON methods for *Array and *Document

3efbeda

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

KrishnaSindhur requested a review from AlekSi as a code owner December 18, 2024 09:49

mergify bot assigned KrishnaSindhur Dec 18, 2024

AlekSi requested changes Dec 18, 2024

View reviewed changes

wirebson/document.go Outdated Show resolved Hide resolved

update: MarshalJSON methods *Document to keep order

cbfdd87

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

KrishnaSindhur requested a review from AlekSi December 18, 2024 15:21

AlekSi requested a review from Copilot December 19, 2024 05:14

Copilot AI reviewed Dec 19, 2024

View reviewed changes

AlekSi reviewed Dec 19, 2024

View reviewed changes

update: added benchmark, fuzzing to bson_test

9c0d986

Signed-off-by: krishna sindhur <krishna.sindhur@thinkbyte.ai>

KrishnaSindhur requested a review from AlekSi December 19, 2024 07:19

AlekSi reviewed Dec 19, 2024

View reviewed changes

krishna sindhur added 2 commits December 19, 2024 13:49

update: error logs

08f70e6

update: test

f2c66b4

KrishnaSindhur requested a review from AlekSi December 19, 2024 09:05

AlekSi requested changes Dec 19, 2024

View reviewed changes

krishna sindhur added 2 commits December 19, 2024 18:16

update: testing for *Documents in bson_test

96860c7

update: testing for *Documents in bson_test

ac72f1f

KrishnaSindhur requested a review from AlekSi December 20, 2024 07:37

AlekSi requested changes Dec 20, 2024

View reviewed changes

update: testing for *Documents in bson_test

7336da4

KrishnaSindhur requested a review from AlekSi December 21, 2024 03:53

AlekSi requested changes Dec 23, 2024

View reviewed changes

AlekSi mentioned this pull request Dec 25, 2024

Add JSON marshalling and unmarshalling #71

Merged

AlekSi closed this in #71 Dec 25, 2024

		jsonObject := make([]byte, 0)
		jsonObject = append(jsonObject, '{')

	jsonObject := make([]byte, 0)
	jsonObject = append(jsonObject, '{')
	res := []byte{'{'}

		// MarshalJSON implements the json.Marshaler interface for Document.
		// It converts the Document into a JSON object representation while preserving the order of fields.

add: MarshalJSON methods for *Array and *Document #70

add: MarshalJSON methods for *Array and *Document #70

Uh oh!

Conversation

KrishnaSindhur commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

AlekSi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrishnaSindhur Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekSi Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KrishnaSindhur Dec 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekSi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekSi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekSi commented Dec 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

add: MarshalJSON methods for Array and Document #70

add: MarshalJSON methods for Array and Document #70

KrishnaSindhur commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 19, 2024 •

edited

Loading

KrishnaSindhur Dec 19, 2024 •

edited

Loading

AlekSi Dec 19, 2024 •

edited

Loading

KrishnaSindhur Dec 19, 2024 •

edited

Loading