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

+	str, truncated := bsoncore.Document(msg).StringN(int(width))

-	// 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?

We could use named return values.

E.g.

func (d Document) StringN(n int) (val string, truncated bool)

Edit: Note that named return values are only generally a good idea on short functions (<50 lines). Disregard this suggestion if StringN becomes longer than 50 lines.

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.

I have a slight preference for -1 (i.e. all negative) vs 0 (i.e. all non-positive) because it more closely matches the semantics of string truncation (i.e. StringN(0) == String()[:0]), although there's not a clear use case for passing either -1 or 0.

Is there a reason we want to change the behavior from the original where we pass math.IntMax to get the full document?

-1 (i.e. all negative) vs 0 (i.e. all non-positive) seems a more sensible interface.

It's just for a better compatibility that not using a particular value like math.IntMax to get the entire document.

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

+	str, truncated := v.stringN(n)
 	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().

matthewdale · 2025-07-29T05:38:06Z

x/bsonx/bsoncore/array_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) {


Optional: Consider factoring these into two test functions TestArray_String and TestArray_StringN and moving the test cases to a package-level variable (e.g. var arrayStringTestCases = ...).

matthewdale · 2025-07-29T05:40:57Z

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) {


Optional: Consider factoring these into two test functions TestDocument_String and TestDocument_StringN and moving the test cases to a package-level variable (e.g. var documentStringTestCases = ...).

matthewdale · 2025-07-29T05:43:23Z

x/bsonx/bsoncore/value_test.go

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


Optional: Consider factoring these into two test functions TestValue_String and TestValue_StringN and moving the test cases to a package-level variable (e.g. var valueStringTestCases = ...).

matthewdale · 2025-07-29T05:44:44Z

x/bsonx/bsoncore/value_test.go

+		{15, `"𨉟呐㗂越"`},
+		{21, `"𨉟呐㗂越"`},
+	} {
+		t.Run(fmt.Sprintf("StringN multi-byte string n==%d", tc.n), func(t *testing.T) {


Optional: Consider factoring this into a separate test function TestArray_StringN_Multibyte.

matthewdale · 2025-07-29T05:49:52Z

x/bsonx/bsoncore/array.go

 	}

-	return buf.String()
+	return buf.String(), truncated


Do we need to truncate the string in the buffer?

E.g.

bsoncoreutil.Truncate(buf.String())

It's not technically necessary, as the buffer has been truncated previously. However, an extra-truncating sounds like a good idea for an additional layer of security

matthewdale · 2025-07-29T05:50:08Z

x/bsonx/bsoncore/document.go

 	}

-	return buf.String()
+	return buf.String(), truncated


Do we need to truncate the string in the buffer?

E.g.

bsoncoreutil.Truncate(buf.String())

It's not technically necessary, as the buffer has been truncated previously. However, an extra-truncating sounds like a good idea for an additional layer of security

matthewdale

Looks good! 👍

prestonvasquez

Thanks!

This reverts commit a709a1d.

mongodb-drivers-pr-bot bot added the review-priority-low Low Priority PR for Review: within 3 business days 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

qingyang-hu requested review from matthewdale and removed request for matthewdale July 28, 2025 17:26

matthewdale reviewed Jul 29, 2025

View reviewed changes

Update StringN() behavior.

555951f

qingyang-hu force-pushed the godriver3605 branch from 74d2883 to 555951f Compare August 1, 2025 15:07

qingyang-hu requested review from matthewdale and prestonvasquez August 1, 2025 15:54

matthewdale approved these changes Aug 4, 2025

View reviewed changes

Merge branch 'master' into godriver3605

bf1361b

prestonvasquez added the bug label Aug 5, 2025

prestonvasquez approved these changes Aug 6, 2025

View reviewed changes

qingyang-hu merged commit a709a1d into mongodb:master Aug 7, 2025
34 of 37 checks passed

qingyang-hu added a commit that referenced this pull request Aug 7, 2025

Revert "GODRIVER-3605 Refactor StringN (#2128)"

f7e9940

This reverts commit a709a1d.

qingyang-hu added a commit that referenced this pull request Aug 7, 2025

GODRIVER-3605 Refactor StringN (#2128)

d4b1b34

mongodb-drivers-pr-bot bot mentioned this pull request Aug 7, 2025

Merge release/2.2 into master #2161

Merged

GODRIVER-3605 Refactor StringN #2128

GODRIVER-3605 Refactor StringN #2128

Uh oh!

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!

matthewdale Jul 29, 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!

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!

matthewdale Jul 29, 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!

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!

matthewdale left a comment

Choose a reason for hiding this comment

Uh oh!

prestonvasquez left a comment

Choose a reason for hiding this comment

Uh oh!

qingyang-hu commented Jul 15, 2025 •

edited

Loading

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

edited

Loading

matthewdale Jul 29, 2025 •

edited

Loading

qingyang-hu Jul 24, 2025 •

edited

Loading

matthewdale Jul 29, 2025 •

edited

Loading