Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace bson with bson2 in wire #4110

Merged
merged 23 commits into from
Feb 28, 2024
Merged
2 changes: 1 addition & 1 deletion .github/settings.yml
Expand Up @@ -55,7 +55,7 @@ labels:
description: Issues about query pushdown
- name: area/types
color: "#0354F4"
description: Issues about data types, BSON, etc
description: Issues about data types, BSON, wire protocol, etc

# backend
- name: backend/pg
Expand Down
1 change: 0 additions & 1 deletion .golangci.yml
Expand Up @@ -37,7 +37,6 @@ linters-settings:
bson:
files:
- $all
- "!**/internal/wire/*.go"
- "!**/internal/bson2/*_test.go"
deny:
- pkg: github.com/FerretDB/FerretDB/internal/bson$
Expand Down
5 changes: 1 addition & 4 deletions Taskfile.yml
Expand Up @@ -324,10 +324,7 @@ tasks:
desc: "Run unit benchmarks"
cmds:
- go test -list='Benchmark.*' ./...
- go test -count=10 -bench=BenchmarkArray -benchtime={{.BENCH_TIME}} ./internal/bson/ | tee -a new.txt
- go test -count=10 -bench=BenchmarkDocument -benchtime={{.BENCH_TIME}} ./internal/bson/ | tee -a new.txt
- go test -count=10 -bench=BenchmarkArray -benchtime={{.BENCH_TIME}} ./internal/handler/sjson/ | tee -a new.txt
- go test -count=10 -bench=BenchmarkDocument -benchtime={{.BENCH_TIME}} ./internal/handler/sjson/ | tee -a new.txt
- go test -count=10 -bench=BenchmarkDocument -benchtime={{.BENCH_TIME}} ./internal/bson2/ | tee -a new.txt
- bin/benchstat{{exeExt}} old.txt new.txt

# That's not quite correct: https://github.com/golang/go/issues/15513
Expand Down
5 changes: 0 additions & 5 deletions internal/bson2/bson2.go
Expand Up @@ -49,11 +49,6 @@ import (
"github.com/FerretDB/FerretDB/internal/util/lazyerrors"
)

// If true, the usage of float64 NaN values is disallowed.
// They mess up many things too much, starting with simple equality tests.
// But allowing them simplifies fuzzing where we currently compare converted [*types.Document]s.
var noNaN = true

type (
// ScalarType represents a BSON scalar type.
//
Expand Down
20 changes: 0 additions & 20 deletions internal/bson2/bson2_test.go
Expand Up @@ -469,11 +469,6 @@ var decodeTestCases = []decodeTestCase{
}

func TestNormal(t *testing.T) {
prev := noNaN
noNaN = false

t.Cleanup(func() { noNaN = prev })

for _, tc := range normalTestCases {
t.Run(tc.name, func(t *testing.T) {
t.Run("bson", func(t *testing.T) {
Expand Down Expand Up @@ -560,11 +555,6 @@ func TestNormal(t *testing.T) {
}

func TestDecode(t *testing.T) {
prev := noNaN
noNaN = false

t.Cleanup(func() { noNaN = prev })

for _, tc := range decodeTestCases {
if tc.decodeDeepErr == nil {
tc.decodeDeepErr = tc.decodeErr
Expand Down Expand Up @@ -622,11 +612,6 @@ func TestDecode(t *testing.T) {
}

func BenchmarkDocument(b *testing.B) {
prev := noNaN
noNaN = false

b.Cleanup(func() { noNaN = prev })

for _, tc := range normalTestCases {
b.Run(tc.name, func(b *testing.B) {
b.Run("bson", func(b *testing.B) {
Expand Down Expand Up @@ -854,11 +839,6 @@ func testRawDocument(t *testing.T, rawDoc RawDocument) {
}

func FuzzDocument(f *testing.F) {
prev := noNaN
noNaN = false

f.Cleanup(func() { noNaN = prev })

for _, tc := range normalTestCases {
f.Add([]byte(tc.raw))
}
Expand Down
5 changes: 0 additions & 5 deletions internal/bson2/decode.go
Expand Up @@ -16,7 +16,6 @@ package bson2

import (
"encoding/binary"
"math"

"github.com/cristalhq/bson/bsonproto"

Expand Down Expand Up @@ -67,10 +66,6 @@ func decodeScalarField(b []byte, t tag) (v any, size int, err error) {
v = f
size = bsonproto.SizeFloat64

if noNaN && math.IsNaN(f) {
err = lazyerrors.Errorf("got NaN value: %w", ErrDecodeInvalidInput)
}

case tagString:
var s string
s, err = bsonproto.DecodeString(b)
Expand Down
7 changes: 0 additions & 7 deletions internal/bson2/document.go
Expand Up @@ -19,7 +19,6 @@ import (
"encoding/binary"
"errors"
"log/slog"
"math"

"github.com/FerretDB/FerretDB/internal/types"
"github.com/FerretDB/FerretDB/internal/util/iterator"
Expand Down Expand Up @@ -140,12 +139,6 @@ func (doc *Document) add(name string, value any) error {
return lazyerrors.Errorf("%q: %w", name, err)
}

if f, ok := value.(float64); ok {
if noNaN && math.IsNaN(f) {
return lazyerrors.New("invalid float64 value NaN")
}
}

doc.fields = append(doc.fields, field{
name: name,
value: value,
Expand Down
5 changes: 0 additions & 5 deletions internal/bson2/encode.go
Expand Up @@ -17,7 +17,6 @@ package bson2
import (
"bytes"
"fmt"
"math"
"time"

"github.com/cristalhq/bson/bsonproto"
Expand Down Expand Up @@ -117,10 +116,6 @@ func encodeField(buf *bytes.Buffer, name string, v any) error {
func encodeScalarField(buf *bytes.Buffer, name string, v any) error {
switch v := v.(type) {
case float64:
if noNaN && math.IsNaN(v) {
return lazyerrors.Errorf("got NaN value")
}

buf.WriteByte(byte(tagFloat64))
case string:
buf.WriteByte(byte(tagString))
Expand Down
2 changes: 1 addition & 1 deletion internal/bson2/raw_document.go
Expand Up @@ -84,7 +84,7 @@ func (raw RawDocument) decode(mode decodeMode) (*Document, error) {
return nil, lazyerrors.Errorf("len(raw) = %d, l = %d: %w", rl, l, ErrDecodeInvalidInput)
}

res := MakeDocument(1)
res := MakeDocument(0)

offset := 4

Expand Down
37 changes: 0 additions & 37 deletions internal/clientconn/conn.go
Expand Up @@ -243,45 +243,8 @@ func (c *conn) run(ctx context.Context) (err error) {
var reqBody wire.MsgBody
var resHeader *wire.MsgHeader
var resBody wire.MsgBody
var validationErr *wire.ValidationError

reqHeader, reqBody, err = wire.ReadMessage(bufr)
if err != nil && errors.As(err, &validationErr) {
// Currently, we respond with OP_MSG containing an error and don't close the connection.
// That's probably not right. First, we always respond with OP_MSG, even to OP_QUERY.
// Second, we don't know what command it was, if any,
// and if the client could handle returned error for it.
//
// TODO https://github.com/FerretDB/FerretDB/issues/2412

// get protocol error to return correct error document
protoErr := handlererrors.ProtocolError(validationErr)

var res wire.OpMsg
must.NoError(res.SetSections(wire.MakeOpMsgSection(
protoErr.Document(),
)))

b := must.NotFail(res.MarshalBinary())

resHeader = &wire.MsgHeader{
OpCode: reqHeader.OpCode,
RequestID: c.lastRequestID.Add(1),
ResponseTo: reqHeader.RequestID,
MessageLength: int32(wire.MsgHeaderLen + len(b)),
}

if err = wire.WriteMessage(bufw, resHeader, &res); err != nil {
return
}

if err = bufw.Flush(); err != nil {
return
}

continue
}

if err != nil {
return
}
Expand Down
10 changes: 1 addition & 9 deletions internal/handler/handlererrors/error.go
Expand Up @@ -19,7 +19,6 @@ import (
"errors"

"github.com/FerretDB/FerretDB/internal/types"
"github.com/FerretDB/FerretDB/internal/wire"
)

//go:generate ../../../bin/stringer -linecomment -type ErrorCode
Expand Down Expand Up @@ -357,8 +356,7 @@ type ProtoErr interface {
// ProtocolError converts any error to wire protocol error.
//
// Nil panics (it never should be passed),
// *CommandError or *WriteErrors (possibly wrapped) are returned unwrapped,
// *wire.ValidationError (possibly wrapped) is returned as CommandError with BadValue code,
// [*CommandError] or [*WriteErrors] (possibly wrapped) are returned unwrapped,
// any other values (including lazy errors) are returned as CommandError with InternalError code.
func ProtocolError(err error) ProtoErr {
if err == nil {
Expand All @@ -375,12 +373,6 @@ func ProtocolError(err error) ProtoErr {
return writeErr
}

var validationErr *wire.ValidationError
if errors.As(err, &validationErr) {
//nolint:errorlint // only *CommandError could be returned
return NewCommandError(ErrBadValue, err).(*CommandError)
}

//nolint:errorlint // only *CommandError could be returned
return NewCommandError(errInternalError, err).(*CommandError)
}
3 changes: 3 additions & 0 deletions internal/wire/msg_body.go
Expand Up @@ -32,6 +32,9 @@ import (
type MsgBody interface {
msgbody() // seal for sumtype

// check checks if body is valid.
check() error

// UnmarshalBinaryNocopy is a variant of [encoding.BinaryUnmarshaler] that does not have to copy the data.
UnmarshalBinaryNocopy([]byte) error

Expand Down