Skip to content
This repository has been archived by the owner on Mar 9, 2021. It is now read-only.

implement clientview fetch #9

Merged
merged 3 commits into from
Mar 20, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/diffs/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,11 @@ type gsp func() (spec.Spec, error)
func serve(parent *kingpin.Application, sps *string, errs io.Writer) {
kc := parent.Command("serve", "Starts a local diff-server.")
port := kc.Flag("port", "The port to run on").Default("7001").Int()
overrideClientViewURL := parent.Flag("clientview", "URL to always use for client view eg 'https://example.com/clientview'").Default("").String()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: client-view for flag name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit on description:

Force diff-server to use a specific URL for all client view fetches on all accounts. Useful for testing.

kc.Action(func(_ *kingpin.ParseContext) error {
ps := fmt.Sprintf(":%d", *port)
log.Printf("Listening on %s...", ps)
s := servepkg.NewService(*sps, accounts.Accounts())
s := servepkg.NewService(*sps, accounts.Accounts(), *overrideClientViewURL)
http.Handle("/", s)
return http.ListenAndServe(fmt.Sprintf(":%d", *port), nil)
})
Expand Down
2 changes: 1 addition & 1 deletion cmd/diffs/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ func TestServe(t *testing.T) {
expectedResponse string
expectedError string
}{
{"handlePullRequest", `{"baseStateID": "00000000000000000000000000000000", "checksum": "00000000"}`,
{"handlePullRequest", `{"baseStateID": "00000000000000000000000000000000", "checksum": "00000000", "clientID": "clientid"}`,
`{"stateID":"unhmo677duk3vbjpu0f01eusdep2k7ei","patch":[{"op":"remove","path":"/"}],"checksum":"00000000"}`, ""},
}

Expand Down
2 changes: 1 addition & 1 deletion db/handle_pull.go → db/diff.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func fullSync(db *DB, from hash.Hash) ([]kv.Operation, Commit) {
return r, makeCommit(db.Noms(), types.Ref{}, datetime.Epoch, db.noms.WriteValue(m.NomsMap()), types.String(m.Checksum().String()))
}

func (db *DB) HandlePull(from hash.Hash, fromChecksum kv.Checksum) ([]kv.Operation, error) {
func (db *DB) Diff(from hash.Hash, fromChecksum kv.Checksum) ([]kv.Operation, error) {
r := []kv.Operation{}
v := db.Noms().ReadValue(from)
var fc Commit
Expand Down
4 changes: 2 additions & 2 deletions db/handle_pull_test.go → db/diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
"roci.dev/diff-server/kv"
)

func TestHandlePull(t *testing.T) {
func TestDiff(t *testing.T) {
assert := assert.New(t)
db, dir := LoadTempDB(assert)
fmt.Println(dir)
Expand Down Expand Up @@ -208,7 +208,7 @@ func TestHandlePull(t *testing.T) {
t.f()
c, err := kv.ChecksumFromString(fromChecksum)
assert.NoError(err)
r, err := db.HandlePull(fromID, *c)
r, err := db.Diff(fromID, *c)
if t.expectedError == "" {
assert.NoError(err, t.label)
expected, err := json.Marshal(t.expectedDiff)
Expand Down
2 changes: 1 addition & 1 deletion kv/map.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ func (me MapEditor) Get(key string) ([]byte, error) {

// Set sets the value for a given key.
func (me *MapEditor) Set(key string, value []byte) error {
nv, err := nomsjson.FromJSON(bytes.NewReader(value), me.noms, nomsjson.FromOptions{})
nv, err := nomsjson.FromJSON(bytes.NewReader(value), me.noms)
if err != nil {
return err
}
Expand Down
3 changes: 3 additions & 0 deletions serve/accounts/accounts.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,19 @@ var (
ID: "sandbox",
Name: "Sandbox",
Pubkey: nil,
// ClientViewURL: "<url>",
},
serve.Account{
ID: "1",
Name: "Rocicorp",
Pubkey: []byte("-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4DwXA3SHZ7TpzahAgOTRRgblBGxL\ndOHVmZ/J1bgBuuxMZzkassAsUCFCaMNu5HZuFUh98kA1laxZzs78O9EDQw==\n-----END PUBLIC KEY-----"),
// ClientViewURL: "<url>",
},
serve.Account{
ID: "2",
Name: "Turtle Technologies, Inc.",
Pubkey: []byte("-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEwNhpc2KRnxQRq2YETuKJShSC623E\nFFlAf4j2fFDKkEDM0Py9HCd3UDmxLyF7HUv9GC9mA+78HahKQnF8F37jLQ==\n-----END PUBLIC KEY-----"),
// ClientViewURL: "<url>",
},
}
)
Expand Down
47 changes: 47 additions & 0 deletions serve/client_view.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
package serve

import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"

servetypes "roci.dev/diff-server/serve/types"
)

type ClientViewGetter struct {
url string
}

// Get fetches a client view. It returns an error if the response from the data layer doesn't have
// a lastTransactionID.
func (g ClientViewGetter) Get(req servetypes.ClientViewRequest, authToken string) (servetypes.ClientViewResponse, error) {
reqBody, err := json.Marshal(req)
if err != nil {
return servetypes.ClientViewResponse{}, fmt.Errorf("could not marshal ClientViewRequest: %w", err)
}
httpReq, err := http.NewRequest("POST", g.url, bytes.NewReader(reqBody))
if err != nil {
return servetypes.ClientViewResponse{}, fmt.Errorf("could not create client view http request: %w", err)
}
httpReq.Header.Add("Authorization", authToken)
httpResp, err := http.DefaultClient.Do(httpReq)
if err != nil {
return servetypes.ClientViewResponse{}, fmt.Errorf("error sending client view http request: %w", err)
}
if httpResp.StatusCode != http.StatusOK {
return servetypes.ClientViewResponse{}, fmt.Errorf("client view fetch http request returned %s", httpResp.Status)
}
var resp servetypes.ClientViewResponse
var r io.Reader = httpResp.Body
defer httpResp.Body.Close()
err = json.NewDecoder(r).Decode(&resp)
if err != nil {
return servetypes.ClientViewResponse{}, fmt.Errorf("couldnt decode client view response: %w", err)
}
if resp.LastTransactionID == "" {
return servetypes.ClientViewResponse{}, fmt.Errorf("malformed response %v missing lastTransactionID", resp)
}
return resp, nil
}
84 changes: 84 additions & 0 deletions serve/client_view_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package serve

import (
"encoding/json"
"net/http"
"net/http/httptest"
"reflect"
"testing"

"github.com/stretchr/testify/assert"
servetypes "roci.dev/diff-server/serve/types"
)

func TestClientViewGetter_Get(t *testing.T) {
assert := assert.New(t)

type args struct {
}
tests := []struct {
name string
req servetypes.ClientViewRequest
authToken string
respCode int
respBody string
want servetypes.ClientViewResponse
wantErr string
}{
{
"ok",
servetypes.ClientViewRequest{ClientID: "clientid"},
"authtoken",
http.StatusOK,
`{"clientView": "clientview", "lastTransactionID": "ltid"}`,
servetypes.ClientViewResponse{ClientView: []byte("\"clientview\""), LastTransactionID: "ltid"},
"",
},
{
"error",
servetypes.ClientViewRequest{ClientID: "clientid"},
"authtoken",
http.StatusBadRequest,
``,
servetypes.ClientViewResponse{},
"400",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to test more of the error message, probably just the entire one. I know you advised erik against this in a previous review, but the error messages (at least these that the customer will see) are part of the dx of the product. It is important that they are high quality.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i do agree in this case we should match on more.

but in general i dont really see how matching part of or all of an error message has anything to do with quality. i can imagine exactly matching a really crappy whole error string. or partially matching a really high quality error string. exact matching just feels a little brittle to me but nbd.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, what I mean is that they should be high quality and then we should have tests that ensure they stay that way.

},
{
"missing last transaction id",
servetypes.ClientViewRequest{ClientID: "clientid"},
"authtoken",
http.StatusOK,
`{"clientView": "foo", "lastTransactionID": ""}`,
servetypes.ClientViewResponse{},
"lastTransactionID",
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {

server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var reqBody servetypes.ClientViewRequest
err := json.NewDecoder(r.Body).Decode(&reqBody)
assert.NoError(err, tt.name)
assert.Equal(tt.req.ClientID, reqBody.ClientID, tt.name)
assert.Equal(tt.authToken, r.Header.Get("Authorization"), tt.name)
w.WriteHeader(tt.respCode)
w.Write([]byte(tt.respBody))
}))

g := ClientViewGetter{
url: server.URL,
}
got, err := g.Get(tt.req, tt.authToken)
if tt.wantErr == "" {
assert.NoError(err)
} else {
assert.Error(err)
assert.Regexp(tt.wantErr, err.Error(), tt.name)
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("ClientViewGetter.Get() case %s got %v (clientview=%s), want %v (clientview=%s)", tt.name, got, string(got.ClientView), tt.want, string(tt.want.ClientView))
}
})
}
}
2 changes: 1 addition & 1 deletion serve/prod/prod.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ const (
)

var (
svc = serve.NewService("aws:replicant/aa-replicant2", accounts.Accounts())
svc = serve.NewService("aws:replicant/aa-replicant2", accounts.Accounts(), "")
)

func init() {
Expand Down
60 changes: 52 additions & 8 deletions serve/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"bytes"
"compress/gzip"
"encoding/json"
"errors"
"fmt"
"io"
"log"
Expand All @@ -25,23 +26,31 @@ import (
"roci.dev/diff-server/db"
"roci.dev/diff-server/kv"
servetypes "roci.dev/diff-server/serve/types"
nomsjson "roci.dev/diff-server/util/noms/json"
)

type clientViewGetter interface {
Get(req servetypes.ClientViewRequest, authToken string) (servetypes.ClientViewResponse, error)
}

// server is a single Replicant instance. The Replicant service runs many such instances.
type server struct {
router *httprouter.Router
db *db.DB
cvg clientViewGetter
mu sync.Mutex
}

func newServer(cs chunks.ChunkStore, urlPrefix string) (*server, error) {
// cvg may be nil, in which case the server skips the client view request in pull, which is
// useful if you are populating the db directly or in tests.
func newServer(cs chunks.ChunkStore, urlPrefix string, cvg clientViewGetter) (*server, error) {
router := httprouter.New()
noms := datas.NewDatabase(cs)
db, err := db.New(noms)
if err != nil {
return nil, err
}
s := &server{router: router, db: db}
s := &server{router: router, db: db, cvg: cvg}
s.router.POST(fmt.Sprintf("%s/handlePullRequest", urlPrefix), func(rw http.ResponseWriter, req *http.Request, ps httprouter.Params) {
body := bytes.Buffer{}
_, err := io.Copy(&body, req.Body)
Expand All @@ -50,23 +59,40 @@ func newServer(cs chunks.ChunkStore, urlPrefix string) (*server, error) {
serverError(rw, err)
return
}
var hsreq servetypes.PullRequest
err = json.Unmarshal(body.Bytes(), &hsreq)
var preq servetypes.PullRequest
err = json.Unmarshal(body.Bytes(), &preq)
if err != nil {
serverError(rw, err)
return
}

from, ok := hash.MaybeParse(hsreq.BaseStateID)
from, ok := hash.MaybeParse(preq.BaseStateID)
if !ok {
clientError(rw, 400, "Invalid baseStateID")
return
}
fromChecksum, err := kv.ChecksumFromString(hsreq.Checksum)
fromChecksum, err := kv.ChecksumFromString(preq.Checksum)
if err != nil {
clientError(rw, 400, "Invalid checksum")
}
patch, err := s.db.HandlePull(from, *fromChecksum)
if preq.ClientID == "" {
clientError(rw, 400, "Missing ClientID")
return
}
if cvg == nil {
log.Print("WARNING: not fetching new client view: no url provided via account or --clientview")
} else {
var cvgError error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defining the separate error with the special handling deviates from standard practice and makes it a bit more difficult to read. I think factoring out fetching and storing the client view might help.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah its a tradeoff i guess. the nesting of the if elses was starting to get really annoying so i went this route which is less standard but more readable to me. agree factoring fetching and storing would make it more readable.

cvreq := servetypes.ClientViewRequest{ClientID: preq.ClientID}
cvresp, cvgError := cvg.Get(cvreq, "") // TODO fritz pass auth token along
if cvgError == nil {
cvgError = storeNewClientView(db, cvresp.ClientView)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of failing to store the client view, something more serious has gone wrong and we should probably 500 no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

up to us. if we think of pull as 'show me the latest client view you have from the data layer' then no. if we think of pull as 'show me exactly what the data layer has right now' then it should probably error. however it almost always makes sense to do the best with what you have in cases like this, which is why i didn't make it fatal. consider a brand new client pulling for the first time. i'd much rather that client get the latest data the server has than get nothing.

}
if cvgError != nil {
log.Printf("WARNING: got error fetching clientview: %s", cvgError)
}
}

patch, err := s.db.Diff(from, *fromChecksum)
if err != nil {
serverError(rw, err)
return
Expand Down Expand Up @@ -102,6 +128,24 @@ func newServer(cs chunks.ChunkStore, urlPrefix string) (*server, error) {
return s, nil
}

func storeNewClientView(db *db.DB, clientView json.RawMessage) error {
v, err := nomsjson.FromJSON(bytes.NewReader(clientView), db.Noms())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have a TODO anywhere about supporting null in the JSON?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a TODO on a piece of paper that says write a test to make sure that client view of {"key": "null"} and client view of "null" both work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these errors end up as serverError at the call site which means they aren't visible to the user. serverError is really more of like Something went wrong inside Replicache.

For debugging purposes, I think we should return problems with the client view as clientError for now, this way they are immediately visible to customers. Long term they also need to be sent to a console somewhere.

return err
}
nm, ok := v.(types.Map)
if !ok {
return fmt.Errorf("clientview is not a Map, it's a %s", v.Kind().String())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Map isn't a JSON concept, it's a Noms concept. The error should be more like Invalid client view, field data must contain a map". Which probably means you want to validate this higher up before parsing into Noms data.

}
// TODO fritz yes this is inefficient, will fix up Map so we don't have to go
// back and forth. But after it works.
m := kv.NewMapFromNoms(db.Noms(), nm)
if m == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use chk.NotNil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it doesn't look like NewMapFromNoms() is expected to ever fail. In that case, we don't need this check here. (this also indicates a problem with the signatures of Map -- we should only return pointers from functions when returning nil is something that can happen)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suppose it could fail. if it could i would argue against crashing the server. we have talked about this before. suppose we have 1000 customers and there is one map that is bad for some reason, eg we are trying some experimental code on a developer account that goes wrong or a super old customer fired back up and we missed doing something to their map. if we crash on errors in the serving path then it is a potential DOS for all 1000 customers. our service might go down because of one bad piece of data. i have a very strong prefernce to not crash the server because it could be doing other useful things not related to the bad data, like serving good data. i have a strong preference to ERROR so that a developer comes and looks at the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I see your point, but in this case, is there a way that NewMapFromNoms() ever returns nil? It seems like no. In that case, can we change its signature and then remove this branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think the map stuff is going to change anyway but yes.

return errors.New("couldnt create a Map from a Noms Map")
}
return db.PutData(m.NomsMap(), types.String(m.Checksum().String()))
}

func (s *server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
verbose.SetVerbose(true)
log.Println("Handling request: ", r.URL.String())
Expand Down
Loading