feat(storage): json downloads #7158
Conversation
product-auto-label
bot
added
size: l
BrennaEpp
force-pushed
the
json-download
branch
from
b439e92
to
7a6af20
Compare
product-auto-label
bot
added
the
stale: old
storage/http_client.go
Outdated
| call.Header().Set("Range", fmt.Sprintf("bytes=%d-%d", start, params.offset+params.length-1)) | ||
| } | ||
| // We wait to assign conditions here because the generation number can change in between reopen() runs. | ||
| if err := applyConds("NewReader", params.gen, params.conds, call); err != nil { |
There was a problem hiding this comment.
We should probably add a test to verify that preconditions including ifGenerationNotMatch are passed through as expected
There was a problem hiding this comment.
Added an integration test for the conditions on reader
storage/http_client.go
Outdated
|
|
||
| // If a generation hasn't been specified, and this is the first response we get, let's record the | ||
| // generation. In future requests we'll use this generation as a precondition to avoid data races. | ||
| if params.gen < 0 && res.Header.Get("X-Goog-Generation") != "" { |
There was a problem hiding this comment.
Do we need to get this from the header, or is it populated in the apiary response somewhere?
There was a problem hiding this comment.
As we discussed the other day, it indeed must be from the header as apiary doesn't add it explicitly to the response anywhere.
storage/http_client.go
Outdated
| reopen: reopen, | ||
| body: body, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
I'm wondering if it's possible to deduplicate some of this code with what you have for XML. I don't have a great idea of how to do this off the top of my head, but worth thinking about maybe.
There was a problem hiding this comment.
Gave it a go at splitting it up, PTAL
product-auto-label
bot
added
size: xl
| client.config.readAPIWasSet = false | ||
| client.config.useJSONforReads = false | ||
| }() | ||
| } |
There was a problem hiding this comment.
In the !ok case we should return an error probably?
There was a problem hiding this comment.
hm maybe, did we have a plan for running these tests with GRPC too?
product-auto-label
bot
added
stale: extraold
|
|
||
| // TestIntegration_JSONReaderConditions tests only JSON reads as some conditions | ||
| // do not work with XML. | ||
| func TestIntegration_JSONReaderConditions(t *testing.T) { |
storage/http_client.go
Outdated
| reopen := readerReopen(ctx, req.Header, params, s, | ||
| func() (*http.Response, error) { return c.hc.Do(req) }, | ||
| func() error { return setConditionsHeaders(req.Header, params.conds) }, | ||
| func(gen int64) { req.URL.RawQuery = fmt.Sprintf("generation=%d", params.gen) }) |
There was a problem hiding this comment.
Looks like the function gen int64 arg isn't actually needed/used, you can just remove it for clarity (and use params.gen directly for JSON as well).
No description provided.