[Enhancement] Restrict internal response body types (#416)

[Enhancement] Restrict internal response body types

What this PR does / why we need it
Restrict extract method arguments to io.Reader
Make Result.Body a byte slice
Make pagination handling less cryptic
Which issue this PR fixes

Resolves #408
Special notes for your reviewer
The reason why Result.Body is []bytes - we can read and unmarshall it multiple times. Readers are expected to be read once.
Progress:

 lint passing
 pagination unit tests passing
 other unit tests passing (here previous implementation iterations died)
 acceptance tests passing

Reviewed-by: Aloento <None>
Reviewed-by: Anton Kachurin <None>
Reviewed-by: Artem Lifshits <None>

Author: outcatcher

internal/extract/doc.go

@@ -1,3 +1,3 @@
 // Package extract contains functions for extracting JSON results into given structure or slice pointers.
-// Those are wrappers over `json.Marshall` and `json.Unmarshall` functions with additional validation built it
+// Those are wrappers over `json.Marshal` and `json.Unmarshal` functions with additional validation built it
 package extract

internal/extract/json.go

@@ -3,13 +3,13 @@ package extract
 import (
 	"bytes"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
-	"net/http"
 	"reflect"
 )
 
-func intoPtr(body, to interface{}, label string) error {
+func intoPtr(body io.Reader, to interface{}, label string) error {
 	if label == "" {
 		return Into(body, &to)
 	}
@@ -98,6 +98,7 @@ func intoPtr(body, to interface{}, label string) error {
 	return err
 }
 
+// JsonMarshal marshals input to bytes via buffer with disabled HTML escaping.
 func JsonMarshal(t interface{}) ([]byte, error) {
 	buffer := &bytes.Buffer{}
 	enc := json.NewEncoder(buffer)
@@ -106,24 +107,31 @@ func JsonMarshal(t interface{}) ([]byte, error) {
 	return buffer.Bytes(), err
 }
 
-func Into(body interface{}, to interface{}) error {
-	if raw, ok := body.(http.Response); ok {
-		body = raw.Body
+// Into parses input as JSON and convert to a structure.
+func Into(body io.Reader, to interface{}) error {
+	if closer, ok := body.(io.ReadCloser); ok {
+		defer closer.Close()
 	}
 
-	if reader, ok := body.(io.ReadCloser); ok {
-		defer reader.Close()
-		return json.NewDecoder(reader).Decode(to)
-	}
+	// json.NewDecoder(..).Decode() replaced with reading whole body for better
+	// error tracing and debug simplicity
+	// TODO: compare this solution to original one in terms of performance
 
-	// TODO: remove this branch in pager refactoring
-	b, err := JsonMarshal(body)
+	byteBody, err := io.ReadAll(body)
 	if err != nil {
-		return err
+		return fmt.Errorf("error reading from stream: %w", err)
 	}
-	err = json.Unmarshal(b, to)
 
-	return err
+	if len(byteBody) == 0 {
+		return nil // empty body - nothing to extract
+	}
+
+	err = json.Unmarshal(byteBody, to)
+	if err != nil && !errors.Is(err, io.EOF) {
+		return fmt.Errorf("error extracting %s into %T: %w", byteBody, to, err)
+	}
+
+	return nil
 }
 
 func typeCheck(to interface{}, kind reflect.Kind) error {
@@ -140,7 +148,7 @@ func typeCheck(to interface{}, kind reflect.Kind) error {
 }
 
 // IntoStructPtr will unmarshal the given body into the provided Struct.
-func IntoStructPtr(body, to interface{}, label string) error {
+func IntoStructPtr(body io.Reader, to interface{}, label string) error {
 	err := typeCheck(to, reflect.Struct)
 	if err != nil {
 		return err
@@ -150,7 +158,7 @@ func IntoStructPtr(body, to interface{}, label string) error {
 }
 
 // IntoSlicePtr will unmarshal the provided body into the provided Slice.
-func IntoSlicePtr(body, to interface{}, label string) error {
+func IntoSlicePtr(body io.Reader, to interface{}, label string) error {
 	err := typeCheck(to, reflect.Slice)
 	if err != nil {
 		return err

internal/extract/json_test.go

@@ -23,38 +23,48 @@ func randomString(prefix string, n int) string {
 	return prefix + string(bytes)
 }
 
+type localCloser struct {
+	*bytes.Reader
+
+	closed bool
+}
+
+func (lc *localCloser) Close() error {
+	lc.closed = true
+	return nil
+}
+
 func TestInto(t *testing.T) {
 	key := "data_key"
 	value := randomString("v-", 20)
 
 	expected := map[string]string{key: value}
 
-	cases := map[string]interface{}{
-		"map": map[string]string{key: value},
-		"struct": struct {
-			DataKey string `json:"data_key"`
-		}{value},
-		"struct with other fields": struct {
-			DataKey  string `json:"data_key"`
-			DataKey2 string `json:"-"`
-		}{value, "difgljdfgn"},
-		"io.Reader": bytes.NewReader([]byte(fmt.Sprintf(`{ "data_key":  "%s"}`, value))),
-	}
+	t.Run("io.Reader", func(t *testing.T) {
+		t.Parallel()
 
-	for name, source := range cases {
-		source := source // avoid issues with parallel tests
-		expectedValue := expected[key]
+		data := bytes.NewReader([]byte(fmt.Sprintf(`{ "data_key":  "%s"}`, value)))
 
-		t.Run(name, func(t *testing.T) {
-			t.Parallel()
+		actual := make(map[string]string)
+		err := Into(data, &actual)
 
-			actual := make(map[string]string)
-			err := Into(source, &actual)
+		assert.NoError(t, err) // not exiting after one fail
+		assert.EqualValues(t, expected[key], actual[key])
+	})
 
-			assert.NoError(t, err) // not exiting after one fail
-			assert.EqualValues(t, expectedValue, actual[key])
-		})
-	}
+	t.Run("io.ReadCloser", func(t *testing.T) {
+		t.Parallel()
+
+		data := bytes.NewReader([]byte(fmt.Sprintf(`{ "data_key":  "%s"}`, value)))
+		closer := &localCloser{Reader: data}
+
+		actual := make(map[string]string)
+		err := Into(closer, &actual)
+
+		assert.NoError(t, err) // not exiting after one fail
+		assert.EqualValues(t, expected[key], actual[key])
+		assert.True(t, closer.closed)
+	})
 }
 
 type TestDataType struct {

openstack/blockstorage/extensions/quotasets/testing/fixtures.go

@@ -36,41 +36,41 @@ var getExpectedQuotaSet = quotasets.QuotaSet{
 
 var getUsageExpectedJSONBody = `
 {
-	"quota_set" : {
-		"id": "555544443333222211110000ffffeeee",
-		"volumes" : {
-			"in_use": 15,
-			"limit": 16,
-			"reserved": 17
-		},
-		"snapshots" : {
-			"in_use": 18,
-			"limit": 19,
-			"reserved": 20
-		},
-		"gigabytes" : {
-			"in_use": 21,
-			"limit": 22,
-			"reserved": 23
-		},
-		"per_volume_gigabytes" : {
-			"in_use": 24,
-			"limit": 25,
-			"reserved": 26
-		},
-		"backups" : {
-			"in_use": 27,
-			"limit": 28,
-			"reserved": 29
-		},
-		"backup_gigabytes" : {
-			"in_use": 30,
-			"limit": 31,
-			"reserved": 32
-		}
-		}
-	}
-}`
+  "quota_set": {
+    "id": "555544443333222211110000ffffeeee",
+    "volumes": {
+      "in_use": 15,
+      "limit": 16,
+      "reserved": 17
+    },
+    "snapshots": {
+      "in_use": 18,
+      "limit": 19,
+      "reserved": 20
+    },
+    "gigabytes": {
+      "in_use": 21,
+      "limit": 22,
+      "reserved": 23
+    },
+    "per_volume_gigabytes": {
+      "in_use": 24,
+      "limit": 25,
+      "reserved": 26
+    },
+    "backups": {
+      "in_use": 27,
+      "limit": 28,
+      "reserved": 29
+    },
+    "backup_gigabytes": {
+      "in_use": 30,
+      "limit": 31,
+      "reserved": 32
+    }
+  }
+}
+`
 
 var getUsageExpectedQuotaSet = quotasets.QuotaUsageSet{
 	ID:                 FirstTenantID,

openstack/blockstorage/v1/snapshots/results.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/opentelekomcloud/gophertelekomcloud"
+	"github.com/opentelekomcloud/gophertelekomcloud/openstack/common/metadata"
 	"github.com/opentelekomcloud/gophertelekomcloud/pagination"
 )
 
@@ -109,11 +110,7 @@ type UpdateMetadataResult struct {
 
 // ExtractMetadata returns the metadata from a response from snapshots.UpdateMetadata.
 func (r UpdateMetadataResult) ExtractMetadata() (map[string]interface{}, error) {
-	if r.Err != nil {
-		return nil, r.Err
-	}
-	m := r.Body.(map[string]interface{})["metadata"]
-	return m.(map[string]interface{}), nil
+	return metadata.Extract(r.BodyReader())
 }
 
 type commonResult struct {

openstack/blockstorage/v2/snapshots/results.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/opentelekomcloud/gophertelekomcloud"
+	"github.com/opentelekomcloud/gophertelekomcloud/openstack/common/metadata"
 	"github.com/opentelekomcloud/gophertelekomcloud/pagination"
 )
 
@@ -99,11 +100,7 @@ type UpdateMetadataResult struct {
 
 // ExtractMetadata returns the metadata from a response from snapshots.UpdateMetadata.
 func (r UpdateMetadataResult) ExtractMetadata() (map[string]interface{}, error) {
-	if r.Err != nil {
-		return nil, r.Err
-	}
-	m := r.Body.(map[string]interface{})["metadata"]
-	return m.(map[string]interface{}), nil
+	return metadata.Extract(r.BodyReader())
 }
 
 type commonResult struct {

openstack/blockstorage/v3/snapshots/results.go

@@ -5,6 +5,7 @@ import (
 	"time"
 
 	"github.com/opentelekomcloud/gophertelekomcloud"
+	"github.com/opentelekomcloud/gophertelekomcloud/openstack/common/metadata"
 	"github.com/opentelekomcloud/gophertelekomcloud/pagination"
 )
 
@@ -111,11 +112,7 @@ type UpdateMetadataResult struct {
 
 // ExtractMetadata returns the metadata from a response from snapshots.UpdateMetadata.
 func (r UpdateMetadataResult) ExtractMetadata() (map[string]interface{}, error) {
-	if r.Err != nil {
-		return nil, r.Err
-	}
-	m := r.Body.(map[string]interface{})["metadata"]
-	return m.(map[string]interface{}), nil
+	return metadata.Extract(r.BodyReader())
 }
 
 type commonResult struct {

openstack/bms/v2/flavors/results.go

@@ -88,7 +88,7 @@ func (page FlavorPage) IsEmpty() (bool, error) {
 // next page of results.
 func (page FlavorPage) NextPageURL() (string, error) {
 	var res []golangsdk.Link
-	err := extract.IntoSlicePtr(page.Result.Body, &res, "flavors_links")
+	err := extract.IntoSlicePtr(page.Result.BodyReader(), &res, "flavors_links")
 	if err != nil {
 		return "", err
 	}
@@ -99,6 +99,6 @@ func (page FlavorPage) NextPageURL() (string, error) {
 // from the List operation.
 func ExtractFlavors(r pagination.Page) ([]Flavor, error) {
 	var res []Flavor
-	err := extract.IntoSlicePtr(r.(FlavorPage).Result.Body, &res, "flavors")
+	err := extract.IntoSlicePtr(r.(FlavorPage).Result.BodyReader(), &res, "flavors")
 	return res, err
 }

openstack/bms/v2/keypairs/results.go

@@ -33,7 +33,7 @@ func ExtractKeyPairs(r pagination.Page) ([]KeyPair, error) {
 		KeyPair KeyPair `json:"keypair"`
 	}
 
-	err := extract.IntoSlicePtr(r.(KeyPairPage).Result.Body, &res, "keypairs")
+	err := extract.IntoSlicePtr(r.(KeyPairPage).Result.BodyReader(), &res, "keypairs")
 	results := make([]KeyPair, len(res))
 
 	for i, pair := range res {

openstack/bms/v2/nics/results.go

@@ -36,7 +36,7 @@ type NicPage struct {
 func (r NicPage) NextPageURL() (string, error) {
 	var res []golangsdk.Link
 
-	err := extract.IntoSlicePtr(r.Body, &res, "interfaceAttachments_links")
+	err := extract.IntoSlicePtr(r.BodyReader(), &res, "interfaceAttachments_links")
 	if err != nil {
 		return "", err
 	}
@@ -55,6 +55,6 @@ func (r NicPage) IsEmpty() (bool, error) {
 // a generic collection is mapped into a relevant slice.
 func ExtractNics(r pagination.Page) ([]Nic, error) {
 	var res []Nic
-	err := extract.IntoSlicePtr(r.(NicPage).Result.Body, &res, "interfaceAttachments")
+	err := extract.IntoSlicePtr(r.(NicPage).BodyReader(), &res, "interfaceAttachments")
 	return res, err
 }