From 0814e1e59375f633704c315f1ec83eac9c50e7e7 Mon Sep 17 00:00:00 2001 From: Martin Polden Date: Thu, 23 Mar 2023 16:34:46 +0100 Subject: Decouple JSON parsing from Document type --- .../go/internal/vespa/document/dispatcher_test.go | 51 ++++++++++---------- client/go/internal/vespa/document/document.go | 56 ++++++++++++---------- client/go/internal/vespa/document/document_test.go | 24 +++------- client/go/internal/vespa/document/http.go | 5 +- client/go/internal/vespa/document/http_test.go | 27 +++++------ 5 files changed, 78 insertions(+), 85 deletions(-) diff --git a/client/go/internal/vespa/document/dispatcher_test.go b/client/go/internal/vespa/document/dispatcher_test.go index 384097670e1..04e0928f2a3 100644 --- a/client/go/internal/vespa/document/dispatcher_test.go +++ b/client/go/internal/vespa/document/dispatcher_test.go @@ -1,7 +1,6 @@ package document import ( - "encoding/json" "sync" "testing" @@ -43,8 +42,8 @@ func TestDispatcher(t *testing.T) { feeder := &mockFeeder{} dispatcher := NewDispatcher(feeder, 2) docs := []Document{ - {PutId: "id:ns:type::doc1", Fields: json.RawMessage(`{"foo": "123"}`)}, - {PutId: "id:ns:type::doc2", Fields: json.RawMessage(`{"bar": "456"}`)}, + {Id: mustParseId("id:ns:type::doc1"), Operation: OperationPut, Body: []byte(`{"fields":{"foo": "123"}}`)}, + {Id: mustParseId("id:ns:type::doc2"), Operation: OperationPut, Body: []byte(`{"fields":{"bar": "456"}}`)}, } for _, d := range docs { dispatcher.Enqueue(d) @@ -57,19 +56,19 @@ func TestDispatcher(t *testing.T) { func TestDispatcherOrdering(t *testing.T) { feeder := &mockFeeder{} - commonId := "id:ns:type::doc1" + commonId := mustParseId("id:ns:type::doc1") docs := []Document{ - mustParseDocument(Document{PutId: commonId}), - mustParseDocument(Document{PutId: "id:ns:type::doc2"}), - mustParseDocument(Document{PutId: "id:ns:type::doc3"}), - mustParseDocument(Document{PutId: "id:ns:type::doc4"}), - mustParseDocument(Document{UpdateId: commonId}), - mustParseDocument(Document{PutId: "id:ns:type::doc5"}), - mustParseDocument(Document{PutId: "id:ns:type::doc6"}), - mustParseDocument(Document{RemoveId: commonId}), - mustParseDocument(Document{PutId: "id:ns:type::doc7"}), - mustParseDocument(Document{PutId: "id:ns:type::doc8"}), - mustParseDocument(Document{PutId: "id:ns:type::doc9"}), + {Id: commonId, Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc2"), Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc3"), Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc4"), Operation: OperationPut}, + {Id: commonId, Operation: OperationUpdate}, + {Id: mustParseId("id:ns:type::doc5"), Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc6"), Operation: OperationPut}, + {Id: commonId, Operation: OperationRemove}, + {Id: mustParseId("id:ns:type::doc7"), Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc8"), Operation: OperationPut}, + {Id: mustParseId("id:ns:type::doc9"), Operation: OperationPut}, } dispatcher := NewDispatcher(feeder, len(docs)) for _, d := range docs { @@ -79,13 +78,13 @@ func TestDispatcherOrdering(t *testing.T) { var wantDocs []Document for _, d := range docs { - if d.Id.String() == commonId { + if d.Id.Equal(commonId) { wantDocs = append(wantDocs, d) } } var gotDocs []Document for _, d := range feeder.documents { - if d.Id.String() == commonId { + if d.Id.Equal(commonId) { gotDocs = append(gotDocs, d) } } @@ -96,12 +95,12 @@ func TestDispatcherOrdering(t *testing.T) { func TestDispatcherOrderingWithFailures(t *testing.T) { feeder := &mockFeeder{} - commonId := "id:ns:type::doc1" + commonId := mustParseId("id:ns:type::doc1") docs := []Document{ - mustParseDocument(Document{PutId: commonId}), - mustParseDocument(Document{PutId: commonId}), - mustParseDocument(Document{UpdateId: commonId}), // fails - mustParseDocument(Document{RemoveId: commonId}), // fails + {Id: commonId, Operation: OperationPut}, + {Id: commonId, Operation: OperationPut}, + {Id: commonId, Operation: OperationUpdate}, // fails + {Id: commonId, Operation: OperationRemove}, // fails } feeder.failAfterN(2) dispatcher := NewDispatcher(feeder, len(docs)) @@ -116,11 +115,11 @@ func TestDispatcherOrderingWithFailures(t *testing.T) { // Dispatching more documents for same ID fails implicitly feeder.failAfterN(0) dispatcher.start() - dispatcher.Enqueue(mustParseDocument(Document{PutId: commonId})) - dispatcher.Enqueue(mustParseDocument(Document{RemoveId: commonId})) + dispatcher.Enqueue(Document{Id: commonId, Operation: OperationPut}) + dispatcher.Enqueue(Document{Id: commonId, Operation: OperationRemove}) // Other IDs are fine - doc2 := mustParseDocument(Document{PutId: "id:ns:type::doc2"}) - doc3 := mustParseDocument(Document{PutId: "id:ns:type::doc3"}) + doc2 := Document{Id: mustParseId("id:ns:type::doc2"), Operation: OperationPut} + doc3 := Document{Id: mustParseId("id:ns:type::doc3"), Operation: OperationPut} dispatcher.Enqueue(doc2) dispatcher.Enqueue(doc3) dispatcher.Close() diff --git a/client/go/internal/vespa/document/document.go b/client/go/internal/vespa/document/document.go index 6aeafd80005..98cb2d1b6c6 100644 --- a/client/go/internal/vespa/document/document.go +++ b/client/go/internal/vespa/document/document.go @@ -103,11 +103,16 @@ func ParseId(serialized string) (Id, error) { }, nil } -// Document represents a Vespa document, and its operation. +// Document represents a Vespa document operation. type Document struct { Id Id Operation Operation + Condition string + Create bool + Body []byte +} +type jsonDocument struct { IdString string `json:"id"` PutId string `json:"put"` UpdateId string `json:"update"` @@ -117,16 +122,6 @@ type Document struct { Fields json.RawMessage `json:"fields"` } -// Body returns the body part of this document, suitable for sending to the /document/v1 API. -func (d Document) Body() []byte { - jsonObject := `{"fields":` - body := make([]byte, 0, len(jsonObject)+len(d.Fields)+1) - body = append(body, []byte(jsonObject)...) - body = append(body, d.Fields...) - body = append(body, byte('}')) - return body -} - // Decoder decodes documents from a JSON structure which is either an array of objects, or objects separated by newline. type Decoder struct { buf *bufio.Reader @@ -193,14 +188,11 @@ func (d *Decoder) decode() (Document, error) { } return Document{}, err } - doc := Document{} + doc := jsonDocument{} if err := d.dec.Decode(&doc); err != nil { return Document{}, err } - if err := parseDocument(&doc); err != nil { - return Document{}, err - } - return doc, nil + return parseDocument(&doc) } func NewDecoder(r io.Reader) *Decoder { @@ -211,29 +203,43 @@ func NewDecoder(r io.Reader) *Decoder { } } -func parseDocument(d *Document) error { +func parseDocument(d *jsonDocument) (Document, error) { id := "" + var op Operation if d.IdString != "" { - d.Operation = OperationPut + op = OperationPut id = d.IdString } else if d.PutId != "" { - d.Operation = OperationPut + op = OperationPut id = d.PutId } else if d.UpdateId != "" { - d.Operation = OperationUpdate + op = OperationUpdate id = d.UpdateId } else if d.RemoveId != "" { - d.Operation = OperationRemove + op = OperationRemove id = d.RemoveId } else { - return fmt.Errorf("invalid document: missing operation: %v", d) + return Document{}, fmt.Errorf("invalid document: missing operation: %v", d) } docId, err := ParseId(id) if err != nil { - return err + return Document{}, err } - d.Id = docId - return nil + var body []byte + if d.Fields != nil { + jsonObject := `{"fields":` + body = make([]byte, 0, len(jsonObject)+len(d.Fields)+1) + body = append(body, []byte(jsonObject)...) + body = append(body, d.Fields...) + body = append(body, byte('}')) + } + return Document{ + Id: docId, + Operation: op, + Condition: d.Condition, + Create: d.Create, + Body: body, + }, nil } func parseError(value string) error { diff --git a/client/go/internal/vespa/document/document_test.go b/client/go/internal/vespa/document/document_test.go index 478dd795dd8..111b9e37acc 100644 --- a/client/go/internal/vespa/document/document_test.go +++ b/client/go/internal/vespa/document/document_test.go @@ -1,8 +1,6 @@ package document import ( - "bytes" - "encoding/json" "io" "reflect" "strings" @@ -11,11 +9,12 @@ import ( func ptr[T any](v T) *T { return &v } -func mustParseDocument(d Document) Document { - if err := parseDocument(&d); err != nil { +func mustParseId(s string) Id { + id, err := ParseId(s) + if err != nil { panic(err) } - return d + return id } func TestParseId(t *testing.T) { @@ -134,9 +133,9 @@ func testDocumentDecoder(t *testing.T, jsonLike string) { t.Helper() r := NewDecoder(strings.NewReader(jsonLike)) want := []Document{ - mustParseDocument(Document{PutId: "id:ns:type::doc1", Fields: json.RawMessage(`{"foo": "123"}`)}), - mustParseDocument(Document{PutId: "id:ns:type::doc2", Fields: json.RawMessage(`{"bar": "456"}`)}), - mustParseDocument(Document{RemoveId: "id:ns:type::doc1", Fields: json.RawMessage(nil)}), + {Id: mustParseId("id:ns:type::doc1"), Operation: OperationPut, Body: []byte(`{"fields":{"foo": "123"}}`)}, + {Id: mustParseId("id:ns:type::doc2"), Operation: OperationPut, Body: []byte(`{"fields":{"bar": "456"}}`)}, + {Id: mustParseId("id:ns:type::doc1"), Operation: OperationRemove}, } got := []Document{} for { @@ -179,12 +178,3 @@ func TestDocumentDecoder(t *testing.T) { t.Errorf("want error %q, got %q", wantErr, err.Error()) } } - -func TestDocumentBody(t *testing.T) { - doc := Document{Fields: json.RawMessage([]byte(`{"foo": "123"}`))} - got := doc.Body() - want := []byte(`{"fields":{"foo": "123"}}`) - if !bytes.Equal(got, want) { - t.Errorf("got %q, want %q", got, want) - } -} diff --git a/client/go/internal/vespa/document/http.go b/client/go/internal/vespa/document/http.go index 981bad5140d..2e01d4564ab 100644 --- a/client/go/internal/vespa/document/http.go +++ b/client/go/internal/vespa/document/http.go @@ -130,8 +130,7 @@ func (c *Client) Send(document Document) Result { stats.Errors = 1 return Result{Status: StatusError, Err: err} } - body := document.Body() - req, err := http.NewRequest(method, url.String(), bytes.NewReader(body)) + req, err := http.NewRequest(method, url.String(), bytes.NewReader(document.Body)) if err != nil { stats.Errors = 1 return Result{Status: StatusError, Err: err} @@ -146,7 +145,7 @@ func (c *Client) Send(document Document) Result { stats.ResponsesByCode = map[int]int64{ resp.StatusCode: 1, } - stats.BytesSent = int64(len(body)) + stats.BytesSent = int64(len(document.Body)) return c.createResult(document.Id, &stats, resp) } diff --git a/client/go/internal/vespa/document/http_test.go b/client/go/internal/vespa/document/http_test.go index ca59c4c2af0..f02c87730d5 100644 --- a/client/go/internal/vespa/document/http_test.go +++ b/client/go/internal/vespa/document/http_test.go @@ -2,7 +2,6 @@ package document import ( "bytes" - "encoding/json" "fmt" "io" "net/http" @@ -27,9 +26,9 @@ func (c *manualClock) now() time.Time { func TestClientSend(t *testing.T) { docs := []Document{ - mustParseDocument(Document{Create: true, UpdateId: "id:ns:type::doc1", Fields: json.RawMessage(`{"foo": "123"}`)}), - mustParseDocument(Document{Create: true, UpdateId: "id:ns:type::doc2", Fields: json.RawMessage(`{"foo": "456"}`)}), - mustParseDocument(Document{Create: true, UpdateId: "id:ns:type::doc3", Fields: json.RawMessage(`{"baz": "789"}`)}), + {Create: true, Id: mustParseId("id:ns:type::doc1"), Operation: OperationUpdate, Body: []byte(`{"fields":{"foo": "123"}}`)}, + {Create: true, Id: mustParseId("id:ns:type::doc2"), Operation: OperationUpdate, Body: []byte(`{"fields":{"foo": "456"}}`)}, + {Create: true, Id: mustParseId("id:ns:type::doc3"), Operation: OperationUpdate, Body: []byte(`{"fields":{"baz": "789"}}`)}, } httpClient := mock.HTTPClient{} client := NewClient(ClientOptions{ @@ -60,7 +59,7 @@ func TestClientSend(t *testing.T) { if err != nil { t.Fatalf("got unexpected error %q", err) } - wantBody := doc.Body() + wantBody := doc.Body if !bytes.Equal(body, wantBody) { t.Errorf("got r.Body = %q, want %q", string(body), string(wantBody)) } @@ -149,25 +148,25 @@ func TestClientFeedURL(t *testing.T) { url string }{ { - mustParseDocument(Document{ - IdString: "id:ns:type::user", - }), + Document{Id: mustParseId("id:ns:type::user")}, "POST", "https://example.com/document/v1/ns/type/docid/user?foo=ba%2Fr", }, { - mustParseDocument(Document{ - UpdateId: "id:ns:type::user", + Document{ + Id: mustParseId("id:ns:type::user"), + Operation: OperationUpdate, Create: true, Condition: "false", - }), + }, "PUT", "https://example.com/document/v1/ns/type/docid/user?condition=false&create=true&foo=ba%2Fr", }, { - mustParseDocument(Document{ - RemoveId: "id:ns:type::user", - }), + Document{ + Id: mustParseId("id:ns:type::user"), + Operation: OperationRemove, + }, "DELETE", "https://example.com/document/v1/ns/type/docid/user?foo=ba%2Fr", }, -- cgit v1.2.3