GODRIVER-3605 Refactor StringN #2128

qingyang-hu · 2025-07-15T13:45:18Z

Summary

Refactor StringN

Background & Motivation

Originally, Element.StringN() does not truncate strings as expected.
This PR also adds a return flag to StringN() to indicate whether the string has been truncated.

mongodb-drivers-pr-bot · 2025-07-15T14:07:03Z

API Change Report

./v2/x/bsonx/bsoncore

incompatible changes

Array.StringN: changed from func(int) string to func(int) (string, bool)
Document.StringN: changed from func(int) string to func(int) (string, bool)
Element.StringN: changed from func(int) string to func(int) (string, bool)
Value.StringN: changed from func(int) string to func(int) (string, bool)

qingyang-hu · 2025-07-23T22:29:49Z

internal/logger/logger.go


-	// If the last byte is not a closing bracket, then the document was truncated
-	if len(str) > 0 && str[len(str)-1] != '}' {


This line causes GODRIVER-3561

prestonvasquez

Excellent work, Qingyang. Thank you for taking this on 👍

prestonvasquez · 2025-07-24T21:58:24Z

x/bsonx/bsoncore/document.go


-	var buf strings.Builder
+// stringN stringify a document. If N is larger than 0, it will truncate the string to N bytes.


Can we clarify in the method documentation that the second parameter indicates if the stringified document was truncated?

prestonvasquez · 2025-07-24T21:59:46Z

x/bsonx/bsoncore/document.go

-func (d Document) StringN(n int) string {
-	if len(d) < 5 || n <= 0 {
-		return ""
+func (d Document) StringN(n int) (string, bool) {


Can we clarify in the method documentation that the second parameter indicates if the stringified document was truncated?

prestonvasquez · 2025-07-24T22:04:42Z

x/bsonx/bsoncore/document.go

+	if n <= 0 {
+		if l, _, ok := ReadLength(d); !ok || l < 5 {
+			return "", false
+		}
+		return "", true
 	}
+	return d.stringN(n)


It's interesting that calling StringN(0) will return an empty string but stringN(0) will return the entire document. The asymmetry makes the code fairly difficult to read. Should we update StringN so that an N <= 0 argument returns the full document?

func (d Document) String(n int) (string, bool) { if n <= 0 { return d.stringN(0) } return d.stringN(n) }

Ultimately, it's unclear to me why one would call StringN(0) expecting an empty string.

By using stringN(), we can keep the original behavior of StringN(n), which is to return an empty string when n <= 0, while also allowing it to be reused by String(). However, I will be happy to merge stringN() and StringN() if we all accept returning the entire document on StringN(0).

I think that makes sense. Will let @matthewdale opine.

prestonvasquez · 2025-07-24T22:08:18Z

x/bsonx/bsoncore/document.go

+	if !ok || length < 5 {
+		return "", false
+	}
+	length -= (4 /* length bytes */ + 1 /* final null byte */)


[nit] Can we re-write this to use // ?

length := (4 + // length bytes 1) // final null bytes

prestonvasquez · 2025-07-24T22:15:29Z

x/bsonx/bsoncore/document.go

-				truncatedStr := bsoncoreutil.Truncate(str, n-buf.Len())
-				buf.WriteString(truncatedStr)
-
+	for length > 0 && !truncated {


[nit] The loop logic is hard to follow, suggest adding comments. I think the following are sufficient:

determine remaining budget l := n - buf.Len()

If buf.Len() >= n, then mark truncated and break

if !first, then write comma, then decrement l and possibly break

ReadElement, subtract its full length, exit on !ok

Delegate to elem.stringN(l), write its str, record its truncated flag

prestonvasquez · 2025-07-24T22:19:00Z

x/bsonx/bsoncore/document.go

+			l = n - buf.Len()
+		}
+		if !first {
+			buf.WriteByte(',')


Are we sure we should be writing the comma here? Without testing directly it seems unintuitive. What if n=1 or something and there isn't enough room for a comma?

Another issue is that we don't need a comma if the document only contains one element, even if there is enough space.

prestonvasquez · 2025-07-24T22:23:12Z

x/bsonx/bsoncore/document_test.go

+	for _, tc := range testCases {
+		for n := -1; n <= len(tc.want)+1; n++ {
+			t.Run(fmt.Sprintf("StringN %s n==%d", tc.description, n), func(t *testing.T) {
+				got, _ := tc.doc.StringN(n)


We should validate the expected values for the the truncation bool.

prestonvasquez · 2025-07-24T22:24:56Z

x/bsonx/bsoncore/element.go

-func (e Element) StringN(n int) string {
+func (e Element) StringN(n int) (string, bool) {
+	if n <= 0 {
+		return "", len(e) > 0


Consider treating n<=0 as "no limit", see comment on document.StringN().

prestonvasquez · 2025-07-24T22:26:26Z

x/bsonx/bsoncore/value.go

 	if n <= 0 {
-		return ""
+		return "", len(str) > 0


Consider treating n<=0 as "no limit", see comment on document.StringN().

prestonvasquez · 2025-07-24T22:27:50Z

x/bsonx/bsoncore/array.go

-	if lens, _, _ := ReadLength(a); lens < 5 || n <= 0 {
-		return ""
+func (a Array) StringN(n int) (string, bool) {
+	if n <= 0 {


Consider treating n<=0 as "no limit", see comment on document.StringN().

mongodb-drivers-pr-bot bot added the priority-3-low Low Priority PR for Review label Jul 15, 2025

qingyang-hu force-pushed the godriver3605 branch from f7afe10 to 050c014 Compare July 15, 2025 18:59

Refactor StringN

4d17e8b

qingyang-hu force-pushed the godriver3605 branch from f9a5d13 to 4d17e8b Compare July 16, 2025 15:00

fixes

0563a2b

qingyang-hu force-pushed the godriver3605 branch from d2d4214 to 0563a2b Compare July 16, 2025 23:58

qingyang-hu commented Jul 23, 2025

View reviewed changes

qingyang-hu force-pushed the godriver3605 branch 3 times, most recently from acac4b7 to 620794a Compare July 23, 2025 22:48

update unit tests

3d68a75

qingyang-hu force-pushed the godriver3605 branch from 620794a to 3d68a75 Compare July 23, 2025 22:53

qingyang-hu marked this pull request as ready for review July 23, 2025 23:18

qingyang-hu requested a review from a team as a code owner July 23, 2025 23:18

qingyang-hu requested review from matthewdale and prestonvasquez July 23, 2025 23:18

prestonvasquez requested changes Jul 24, 2025

View reviewed changes


		// If the last byte is not a closing bracket, then the document was truncated
		if len(str) > 0 && str[len(str)-1] != '}' {


		var buf strings.Builder
		// stringN stringify a document. If N is larger than 0, it will truncate the string to N bytes.

GODRIVER-3605 Refactor StringN #2128

Are you sure you want to change the base?

GODRIVER-3605 Refactor StringN #2128

Conversation

qingyang-hu commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Background & Motivation

Uh oh!

mongodb-drivers-pr-bot bot commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

API Change Report

./v2/x/bsonx/bsoncore

incompatible changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prestonvasquez 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!

qingyang-hu Jul 24, 2025 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qingyang-hu commented Jul 15, 2025 •

edited

Loading

mongodb-drivers-pr-bot bot commented Jul 15, 2025 •

edited

Loading

qingyang-hu Jul 24, 2025 •

edited

Loading