-
Notifications
You must be signed in to change notification settings - Fork 3
implement clientview fetch #9
Conversation
This pull request is being automatically deployed with ZEIT Now (learn more). 🔍 Inspect: https://zeit.co/rocicorp/diff-server/jm9e9w43x |
@@ -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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
if cvg == nil { | ||
log.Print("WARNING: not fetching new client view: no url provided via account or --clientview") | ||
} else { | ||
var cvgError error |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, "") // auth token TODO | ||
if cvgError == nil { | ||
cvgError = storeNewClientView(db, cvresp.ClientView) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -102,6 +126,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()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@@ -102,6 +126,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()) | |||
if err != nil { |
There was a problem hiding this comment.
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.
} | ||
nm, ok := v.(types.Map) | ||
if !ok { | ||
return fmt.Errorf("clientview is not a Map, it's a %s", v.Kind().String()) |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use chk.NotNil
.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
serve/client_view.go
Outdated
url string | ||
} | ||
|
||
// Get fetches a client view. It returns an error if the last transaction id or state id is missing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are lots of other cases it returns error for, so I'd just summarize by saying: it returns an error if the client view can't be fetched or is invalid.
serve/client_view.go
Outdated
func (g ClientViewGetter) Get(req servetypes.ClientViewRequest, authToken string) (servetypes.ClientViewResponse, error) { | ||
reqBody, err := json.Marshal(req) | ||
if err != nil { | ||
return servetypes.ClientViewResponse{}, errors.Wrap(err, "could not marshal ClientViewRequest") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only example of an error in this function that isn't an issue on customer side. Since we know that if our code is correct, this marshaling will always succeed, you could just chk.NoError()
here. Alternately return two different errors: an internal error and a client error, and call serverError()
on the internal one and clientError()
on the other.
http.StatusBadRequest, | ||
``, | ||
servetypes.ClientViewResponse{}, | ||
"400", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
fyi does not yet pass the last transaction id back in the pullresponse