From ff10e0e85e561c98e5936822c0d5d8af563a32a0 Mon Sep 17 00:00:00 2001 From: raeidish <118611562+raeidish@users.noreply.github.com> Date: Tue, 2 May 2023 15:37:49 +0200 Subject: [PATCH 1/3] Remove version and name assertions in integration tests (#2552) Closes #2500. --- integration/commands_diagnostic_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/integration/commands_diagnostic_test.go b/integration/commands_diagnostic_test.go index ba4b3964a764..ecaea501a875 100644 --- a/integration/commands_diagnostic_test.go +++ b/integration/commands_diagnostic_test.go @@ -16,7 +16,6 @@ package integration import ( "net" - "runtime" "testing" "github.com/stretchr/testify/assert" @@ -222,11 +221,6 @@ func TestCommandsDiagnosticHostInfo(t *testing.T) { os := m["os"].(bson.D) assert.Equal(t, []string{"type", "name", "version"}, CollectKeys(t, os)) - if runtime.GOOS == "linux" { - require.NotEmpty(t, os.Map()["name"], "os name should not be empty") - require.NotEmpty(t, os.Map()["version"], "os version should not be empty") - } - system := m["system"].(bson.D) keys := CollectKeys(t, system) assert.Contains(t, keys, "currentTime") From 30dd84203a10aa5ed21b7245043534cb8ae365cf Mon Sep 17 00:00:00 2001 From: Alexey Palazhchenko Date: Wed, 3 May 2023 14:47:35 +0400 Subject: [PATCH 2/3] Add helpers for iterators and generators (#2542) --- internal/util/iterator/count.go | 42 +++++++++++++ internal/util/iterator/func.go | 79 +++++++++++++++++++++++++ internal/util/iterator/iterator.go | 2 +- internal/util/iterator/iterator_test.go | 36 +++++++++++ internal/util/iterator/slice.go | 8 ++- internal/util/iterator/values.go | 1 + internal/util/iterator/with_close.go | 15 ++--- 7 files changed, 172 insertions(+), 11 deletions(-) create mode 100644 internal/util/iterator/count.go create mode 100644 internal/util/iterator/func.go diff --git a/internal/util/iterator/count.go b/internal/util/iterator/count.go new file mode 100644 index 000000000000..ecb67e4f0d2f --- /dev/null +++ b/internal/util/iterator/count.go @@ -0,0 +1,42 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iterator + +import "errors" + +// ConsumeCount returns the number of elements in the iterator. +// ErrIteratorDone error is returned as nil; any other error is returned as-is. +// +// Iterator is always closed at the end. +func ConsumeCount[K, V any](iter Interface[K, V]) (int, error) { + defer iter.Close() + + var count int + var err error + + for { + _, _, err = iter.Next() + if err == nil { + count++ + continue + } + + if errors.Is(err, ErrIteratorDone) { + err = nil + } + + return count, err + } +} diff --git a/internal/util/iterator/func.go b/internal/util/iterator/func.go new file mode 100644 index 000000000000..42db78acbd25 --- /dev/null +++ b/internal/util/iterator/func.go @@ -0,0 +1,79 @@ +// Copyright 2021 FerretDB Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package iterator + +import ( + "fmt" + "sync" + + "github.com/FerretDB/FerretDB/internal/util/resource" +) + +// NextFunc is a part of Interface for the Next method. +type NextFunc[K, V any] func() (K, V, error) + +// valuesIterator implements iterator.Interface. +// +//nolint:vet // for readability +type funcIterator[K, V any] struct { + m sync.Mutex + f NextFunc[K, V] + + token *resource.Token +} + +// ForFunc returns an iterator for the given function. +func ForFunc[K, V any](f NextFunc[K, V]) Interface[K, V] { + iter := &funcIterator[K, V]{ + f: f, + token: resource.NewToken(), + } + + resource.Track(iter, iter.token) + + return iter +} + +// Next implements iterator.Interface. +func (iter *funcIterator[K, V]) Next() (K, V, error) { + iter.m.Lock() + defer iter.m.Unlock() + + if iter.f == nil { + var k K + var v V + + return k, v, fmt.Errorf("%w (f is nil)", ErrIteratorDone) + } + + return iter.f() +} + +// Close implements iterator.Interface. +func (iter *funcIterator[K, V]) Close() { + iter.m.Lock() + defer iter.m.Unlock() + + iter.f = nil + + resource.Untrack(iter, iter.token) +} + +// check interfaces +var ( + _ Interface[any, any] = (*funcIterator[any, any])(nil) + _ NextFunc[any, any] = (*funcIterator[any, any])(nil).Next + _ Closer = (*funcIterator[any, any])(nil) +) diff --git a/internal/util/iterator/iterator.go b/internal/util/iterator/iterator.go index 805e5f03e0b8..e10883f6fe64 100644 --- a/internal/util/iterator/iterator.go +++ b/internal/util/iterator/iterator.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package iterator describes a generic Iterator interface. +// Package iterator describes a generic Iterator interface and related utilities. package iterator import "errors" diff --git a/internal/util/iterator/iterator_test.go b/internal/util/iterator/iterator_test.go index 13fce12f878f..76d2c5af4542 100644 --- a/internal/util/iterator/iterator_test.go +++ b/internal/util/iterator/iterator_test.go @@ -54,3 +54,39 @@ func TestConsumeValuesN(t *testing.T) { require.NoError(t, err) assert.Nil(t, actual) } + +func TestForFunc(t *testing.T) { + var i int + f := func() (struct{}, int, error) { + var k struct{} + + i++ + if i > 3 { + return k, 0, ErrIteratorDone + } + + return k, i, nil + } + + iter1 := ForFunc(f) + iter2 := ForFunc(f) + + defer iter1.Close() + defer iter2.Close() + + _, v, err := iter1.Next() + require.NoError(t, err) + assert.Equal(t, 1, v) + + _, v, err = iter2.Next() + require.NoError(t, err) + assert.Equal(t, 2, v) + + _, v, err = iter1.Next() + require.NoError(t, err) + assert.Equal(t, 3, v) + + _, v, err = iter2.Next() + require.ErrorIs(t, err, ErrIteratorDone) + assert.Equal(t, 0, v) +} diff --git a/internal/util/iterator/slice.go b/internal/util/iterator/slice.go index 0f6269ddd0ac..9ca84d886d41 100644 --- a/internal/util/iterator/slice.go +++ b/internal/util/iterator/slice.go @@ -15,6 +15,7 @@ package iterator import ( + "fmt" "sync" "github.com/FerretDB/FerretDB/internal/util/resource" @@ -50,9 +51,9 @@ func (iter *sliceIterator[V]) Next() (int, V, error) { iter.n++ n := int(iter.n) - 1 - var zero V - if n >= len(iter.s) { - return 0, zero, ErrIteratorDone + if l := len(iter.s); n >= l { + var v V + return 0, v, fmt.Errorf("%w (n (%d) >= len (%d)", ErrIteratorDone, n, l) } return n, iter.s[n], nil @@ -71,5 +72,6 @@ func (iter *sliceIterator[V]) Close() { // check interfaces var ( _ Interface[int, any] = (*sliceIterator[any])(nil) + _ NextFunc[int, any] = (*sliceIterator[any])(nil).Next _ Closer = (*sliceIterator[any])(nil) ) diff --git a/internal/util/iterator/values.go b/internal/util/iterator/values.go index a9584059f8ed..51d86215562b 100644 --- a/internal/util/iterator/values.go +++ b/internal/util/iterator/values.go @@ -105,5 +105,6 @@ func (iter *valuesIterator[K, V]) Close() { // check interfaces var ( _ Interface[struct{}, any] = (*valuesIterator[any, any])(nil) + _ NextFunc[struct{}, any] = (*valuesIterator[any, any])(nil).Next _ Closer = (*valuesIterator[any, any])(nil) ) diff --git a/internal/util/iterator/with_close.go b/internal/util/iterator/with_close.go index 51125cf3321f..812393cf151d 100644 --- a/internal/util/iterator/with_close.go +++ b/internal/util/iterator/with_close.go @@ -14,8 +14,8 @@ package iterator -// withClose wraps an iterator with a custom close function. -type withClose[K, V any] struct { +// withCloseIterator wraps an iterator with a custom close function. +type withCloseIterator[K, V any] struct { iter Interface[K, V] close func() } @@ -24,25 +24,26 @@ type withClose[K, V any] struct { // // That function should call Close() method of the wrapped iterator. func WithClose[K, V any](iter Interface[K, V], close func()) Interface[K, V] { - return &withClose[K, V]{ + return &withCloseIterator[K, V]{ iter: iter, close: close, } } // Next implements iterator.Interface by calling Next() method of the wrapped iterator. -func (iter *withClose[K, V]) Next() (K, V, error) { +func (iter *withCloseIterator[K, V]) Next() (K, V, error) { return iter.iter.Next() } // Close implements iterator.Interface by calling the provided close function. -func (iter *withClose[K, V]) Close() { +func (iter *withCloseIterator[K, V]) Close() { // we might want to wrap it with sync.Once if needed iter.close() } // check interfaces var ( - _ Interface[any, any] = (*withClose[any, any])(nil) - _ Closer = (*withClose[any, any])(nil) + _ Interface[any, any] = (*withCloseIterator[any, any])(nil) + _ NextFunc[any, any] = (*withCloseIterator[any, any])(nil).Next + _ Closer = (*withCloseIterator[any, any])(nil) ) From 717f0fa34d006074cabec360712c929d63a8fe60 Mon Sep 17 00:00:00 2001 From: Alexey Palazhchenko Date: Wed, 3 May 2023 15:16:02 +0400 Subject: [PATCH 3/3] Do various small cleanups (#2561) --- integration/Taskfile.yml | 2 +- internal/handlers/common/params.go | 37 ++++++++++++++++++++++-------- internal/types/document.go | 9 ++++++++ internal/util/iterator/count.go | 2 +- internal/util/iterator/values.go | 2 +- internal/util/must/must.go | 13 +++++++++++ tools/Taskfile.yml | 2 +- 7 files changed, 53 insertions(+), 14 deletions(-) diff --git a/integration/Taskfile.yml b/integration/Taskfile.yml index f104d12f3373..58e806d9f014 100644 --- a/integration/Taskfile.yml +++ b/integration/Taskfile.yml @@ -5,7 +5,7 @@ version: 3 vars: BENCHTIME: 5s BENCHNAME: '{{default "." .BENCHNAME}}' - RACEFLAG: -race={{ne OS "windows"}} + RACEFLAG: -race={{and (ne OS "windows") (ne ARCH "arm") (ne ARCH "riscv64")}} tasks: env-data: diff --git a/internal/handlers/common/params.go b/internal/handlers/common/params.go index 53f7095c321b..1d95ecb7c829 100644 --- a/internal/handlers/common/params.go +++ b/internal/handlers/common/params.go @@ -25,12 +25,13 @@ import ( "github.com/FerretDB/FerretDB/internal/util/must" ) -// GetRequiredParam returns doc's value for key or protocol error for missing or invalid parameter. +// GetRequiredParam returns doc's value for key +// or protocol error for missing key or invalid type. func GetRequiredParam[T types.Type](doc *types.Document, key string) (T, error) { var zero T - v, err := doc.Get(key) - if err != nil { + v, _ := doc.Get(key) + if v == nil { msg := fmt.Sprintf("required parameter %q is missing", key) return zero, commonerrors.NewCommandErrorMsgWithArgument(commonerrors.ErrBadValue, msg, key) } @@ -44,13 +45,15 @@ func GetRequiredParam[T types.Type](doc *types.Document, key string) (T, error) return res, nil } -// GetOptionalParam returns doc's value for key, default value for missing parameter, or protocol error for invalid parameter. +// GetOptionalParam returns doc's value for key, default value for missing parameter, +// or protocol error for invalid type. func GetOptionalParam[T types.Type](doc *types.Document, key string, defaultValue T) (T, error) { - v, err := doc.Get(key) - if err != nil { + v, _ := doc.Get(key) + if v == nil { return defaultValue, nil } + // require exact type; do not threat nulls as default values in this helper res, ok := v.(T) if !ok { msg := fmt.Sprintf( @@ -64,13 +67,27 @@ func GetOptionalParam[T types.Type](doc *types.Document, key string, defaultValu return res, nil } +// GetOptionalNullParam returns doc's value for key, default value for missing parameter or null, +// or protocol error for other invalid type. +func GetOptionalNullParam[T types.Type](doc *types.Document, key string, defaultValue T) (T, error) { + v, err := GetOptionalParam(doc, key, defaultValue) + if err != nil { + // the only possible error here is type mismatch, so the key is present + if _, ok := must.NotFail(doc.Get(key)).(types.NullType); ok { + err = nil + } + } + + return v, err +} + // GetBoolOptionalParam returns doc's bool value for key. // Non-zero double, long, and int values return true. // Zero values for those types, as well as nulls and missing fields, return false. // Other types return a protocol error. func GetBoolOptionalParam(doc *types.Document, key string) (bool, error) { - v, err := doc.Get(key) - if err != nil { + v, _ := doc.Get(key) + if v == nil { return false, nil } @@ -495,8 +512,8 @@ func multiplyLongSafely(v1, v2 int64) (int64, error) { // GetOptionalPositiveNumber returns doc's value for key or protocol error for invalid parameter. func GetOptionalPositiveNumber(document *types.Document, key string) (int32, error) { - v, err := document.Get(key) - if err != nil { + v, _ := document.Get(key) + if v == nil { return 0, nil } diff --git a/internal/types/document.go b/internal/types/document.go index ce6761fd62dc..2bc60b1526b6 100644 --- a/internal/types/document.go +++ b/internal/types/document.go @@ -236,6 +236,15 @@ func (d *Document) Has(key string) bool { // Get returns a value at the given key. // If the key is duplicated, it panics. +// +// The only possible error is returned for a missing key. +// In that case, Get returns nil for the value. +// The new code is encouraged to do this: +// +// v, _ := d.Get("key") +// if v == nil { ... } +// +// The error value will be removed in the future. func (d *Document) Get(key string) (any, error) { if d.isKeyDuplicate(key) { panic(fmt.Sprintf("types.Document.Get: key is duplicated: %s", key)) diff --git a/internal/util/iterator/count.go b/internal/util/iterator/count.go index ecb67e4f0d2f..7bfa688ab368 100644 --- a/internal/util/iterator/count.go +++ b/internal/util/iterator/count.go @@ -19,7 +19,7 @@ import "errors" // ConsumeCount returns the number of elements in the iterator. // ErrIteratorDone error is returned as nil; any other error is returned as-is. // -// Iterator is always closed at the end. +// It always closes the iterator. func ConsumeCount[K, V any](iter Interface[K, V]) (int, error) { defer iter.Close() diff --git a/internal/util/iterator/values.go b/internal/util/iterator/values.go index 51d86215562b..d00590f3ed3c 100644 --- a/internal/util/iterator/values.go +++ b/internal/util/iterator/values.go @@ -23,7 +23,7 @@ import ( // ConsumeValues consumes all values from iterator until it is done. // ErrIteratorDone error is returned as nil; any other error is returned as-is. // -// Iterator is always closed at the end. +// It always closes the iterator. func ConsumeValues[K, V any](iter Interface[K, V]) ([]V, error) { defer iter.Close() diff --git a/internal/util/must/must.go b/internal/util/must/must.go index e29aee547f09..3eccb8dc4252 100644 --- a/internal/util/must/must.go +++ b/internal/util/must/must.go @@ -15,6 +15,8 @@ // Package must provides helper functions that panic on error. package must +import "fmt" + // NotFail panics if the error is not nil, returns res otherwise. // // Use that function only for static initialization, test code, or code that "can't" fail. @@ -36,6 +38,17 @@ func NoError(err error) { } } +// NotBeZero panics if argument has zero value. +// +// Use that function only for static initialization, test code, or code that "can't" fail. +// When in doubt, don't. +func NotBeZero[T comparable](v T) { + var zero T + if v == zero { + panic(fmt.Sprintf("v has zero value (%#v)", v)) + } +} + // BeTrue panic if the b is not true. // // Use that function only for static initialization, test code, or statemants that diff --git a/tools/Taskfile.yml b/tools/Taskfile.yml index c474eeaf6421..3c37afe51c1a 100644 --- a/tools/Taskfile.yml +++ b/tools/Taskfile.yml @@ -3,7 +3,7 @@ version: 3 vars: - RACEFLAG: -race={{ne OS "windows"}} + RACEFLAG: -race={{and (ne OS "windows") (ne ARCH "arm") (ne ARCH "riscv64")}} tasks: tools-test: