What the developer wrote
Authored by Gijs van Dam
fn: fix Result[T].FlatMap
For a Result[T], FlatMap should apply f when the result is Ok, and
propagate the error unchanged when it's Err. The original code returns r
on Ok and tries to use r.left when Err, which is wrong. This commit
fixes that.
Secondly, the group of FlatMap/AndThen and OrElse functions and methods
are now properly tested with new unit tests.
fixes #10401
What changed, and why it matters
This commit fixes a logic bug in a small helper function called FlatMap used inside the LND codebase. FlatMap is supposed to transform a successful value and pass through failures unchanged. The old code did the opposite: it left successes alone and tried to transform failures, which could cause errors to be silently mishandled or successful values to be lost. The commit also adds unit tests to prevent the bug from returning.
Review all call sites of FlatMap, AndThen, and FlatMapResult in the LND codebase to determine whether the inverted behavior caused incorrect state transitions, skipped validations, or mishandled errors. Run the new unit tests and consider backporting the fix to active release branches.
Security signals we found
Logic inversion in error-handling primitive
Potential for error values to be passed where success values are expected
Potential for successful results to bypass intended validation/transformation
Fix is accompanied by new unit tests
Evidence from the diff
In fn/result.go, Result[T].FlatMap had its two branches swapped. The corrected behavior is: if r.IsOk(), return f(r.left) (apply the continuation to the success value); otherwise return r (propagate the error). The bug meant callers of FlatMap/AndThen/FlatMapResult would not execute the intended transformation on success and would invoke the continuation with an error value on failure. The patch is a two-line swap plus 129 lines of new tests covering FlatMap, AndThen, OrElse, FlatMapResult, and AndThen func variants.
Changed components
fn/result.go: Result[T].FlatMapfn/result.go: AndThen method/func (alias for FlatMap/FlatMapResult)fn/result.go: FlatMapResultAny downstream LND code using these helpersInspect captured patch +129 / −2
diff --git a/fn/result.go b/fn/result.go
index 37958f2..39f8674 100644
--- a/fn/result.go
+++ b/fn/result.go
@@ -149,10 +149,10 @@ func FlattenResult[A any](r Result[Result[A]]) Result[A] {
// success value if it exists.
func (r Result[T]) FlatMap(f func(T) Result[T]) Result[T] {
if r.IsOk() {
- return r
+ return f(r.left)
}
- return f(r.left)
+ return r
}
// AndThen is an alias for FlatMap. This along with OrElse can be used to
diff --git a/fn/result_test.go b/fn/result_test.go
index 2b5d942..7a4f86a 100644
--- a/fn/result_test.go
+++ b/fn/result_test.go
@@ -96,3 +96,130 @@ func TestSinkOnOkContinuationCall(t *testing.T) {
require.True(t, called)
require.Nil(t, res)
}
+
+var errFlatMap = errors.New("fail")
+var errFlatMapOrig = errors.New("original")
+
+var flatMapTestCases = []struct {
+ name string
+ input Result[int]
+ fnA func(int) Result[int]
+ fnB func(int) Result[string]
+ expectedA Result[int]
+ expectedB Result[string]
+}{
+ {
+ name: "Ok to Ok",
+ input: Ok(1),
+ fnA: func(i int) Result[int] { return Ok(i + 1) },
+ fnB: func(i int) Result[string] {
+ return Ok(fmt.Sprintf("%d", i+1))
+ },
+ expectedA: Ok(2),
+ expectedB: Ok("2"),
+ },
+ {
+ name: "Ok to Err",
+ input: Ok(1),
+ fnA: func(i int) Result[int] {
+ return Err[int](errFlatMap)
+ },
+ fnB: func(i int) Result[string] {
+ return Err[string](errFlatMap)
+ },
+ expectedA: Err[int](errFlatMap),
+ expectedB: Err[string](errFlatMap),
+ },
+ {
+ name: "Err to Err (function not called)",
+ input: Err[int](errFlatMapOrig),
+ fnA: func(i int) Result[int] { return Ok(i + 1) },
+ fnB: func(i int) Result[string] {
+ return Ok("should not happen")
+ },
+ expectedA: Err[int](errFlatMapOrig),
+ expectedB: Err[string](errFlatMapOrig),
+ },
+}
+
+var orElseTestCases = []struct {
+ name string
+ input Result[int]
+ fn func(error) Result[int]
+ expected Result[int]
+}{
+ {
+ name: "Ok to Ok (function not called)",
+ input: Ok(1),
+ fn: func(err error) Result[int] { return Ok(2) },
+ expected: Ok(1),
+ },
+ {
+ name: "Err to Ok",
+ input: Err[int](errFlatMapOrig),
+ fn: func(err error) Result[int] { return Ok(2) },
+ expected: Ok(2),
+ },
+ {
+ name: "Err to Err",
+ input: Err[int](errFlatMapOrig),
+ fn: func(err error) Result[int] {
+ return Err[int](errFlatMap)
+ },
+ expected: Err[int](errFlatMap),
+ },
+}
+
+func TestFlatMap(t *testing.T) {
+ for _, tc := range flatMapTestCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+ actual := tc.input.FlatMap(tc.fnA)
+ require.Equal(t, tc.expectedA, actual)
+ })
+ }
+}
+
+func TestAndThenMethod(t *testing.T) {
+ // Since AndThen is just an alias for FlatMap, we can reuse the same
+ // test cases.
+ for _, tc := range flatMapTestCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+ actual := tc.input.AndThen(tc.fnA)
+ require.Equal(t, tc.expectedA, actual)
+ })
+ }
+}
+
+func TestOrElseMethod(t *testing.T) {
+ for _, tc := range orElseTestCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+ actual := tc.input.OrElse(tc.fn)
+ require.Equal(t, tc.expected, actual)
+ })
+ }
+}
+
+func TestFlatMapResult(t *testing.T) {
+ for _, tc := range flatMapTestCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+ actual := FlatMapResult(tc.input, tc.fnB)
+ require.Equal(t, tc.expectedB, actual)
+ })
+ }
+}
+
+func TestAndThenFunc(t *testing.T) {
+ // Since AndThen is just an alias for FlatMapResult, we can reuse the
+ // same test cases.
+ for _, tc := range flatMapTestCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+ actual := AndThen(tc.input, tc.fnB)
+ require.Equal(t, tc.expectedB, actual)
+ })
+ }
+}
Why this scored 36/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.