jhump commented on code in PR #384:
URL: https://github.com/apache/iceberg-go/pull/384#discussion_r2045891208
##########
catalog/rest/rest.go:
##########
@@ -221,12 +222,12 @@ func (s *sessionTransport) RoundTrip(r *http.Request)
(*http.Response, error) {
return nil, err
}
- if _, err = io.Copy(s.h, rdr); err != nil {
+ h := s.newHash()
+ if _, err = io.Copy(h, rdr); err != nil {
return nil, err
}
- payloadHash = string(s.h.Sum(nil))
- s.h.Reset()
+ payloadHash = hex.EncodeToString(h.Sum(nil))
Review Comment:
Oops -- really need an integration test of some sort for this.
Unfortunately, the AWS Go library doesn't seem to include a signature verifier
that we can easily drop into a test server to make sure these are correct. But
after stepping through the new test, I _think_ it should all work, with these
fixes in.
##########
catalog/rest/rest.go:
##########
@@ -375,10 +376,7 @@ func handleNon200(rsp *http.Response, override
map[int]error) error {
return e
}
-func fromProps(props iceberg.Properties) *options {
- o := &options{
- additionalProps: iceberg.Properties{},
- }
+func fromProps(props iceberg.Properties, o *options) {
Review Comment:
I changed the signature to make it less error-prone to round-trip the
options into properties and back. This is done when creating a client, to
convert options to properties, then merge in any properties retrieved from the
server, and then convert back to options.
This way, we can easily clone the original options object and then override
values from the properties. Previously, any time a new field was added to
options that was not represented in properties (like the new `awsConfigSet`
flag I added), if one forgot to update the list of manually copied fields
([here](https://github.com/apache/iceberg-go/pull/384/files#diff-b4f7aa1f4862334f1387eca97a977f5bcc7ba52a527562ad2f89cc4c767d618cL631-L632)),
it would be easy to introduce a subtle bug.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
