From 8dac72f4fd3fd1f57655bdf65f21975b95e546c3 Mon Sep 17 00:00:00 2001 From: Martin Sehnoutka Date: Thu, 16 Jan 2020 15:31:22 +0100 Subject: [PATCH] internal: propose custom types for image types, arches, etc. We currently use strings for passing arches and image types around, which is not ideal. We should have a finite set of supported image types and architectures as well as upload targets. This PR introduces custom types to make the code base more readable and possibly also more correct. I considered some alternatives like a struct with private fields, but struct cannot be const, so that does not help either. Eventually I think this is the "get s**t done" solution. The package also includes unit-tests which try to convert string to structure and the other way around to make sure it all works properly. --- internal/common/types.go | 212 ++++++++++++++++++++++++++++++++++ internal/common/types_test.go | 101 ++++++++++++++++ 2 files changed, 313 insertions(+) create mode 100644 internal/common/types.go create mode 100644 internal/common/types_test.go diff --git a/internal/common/types.go b/internal/common/types.go new file mode 100644 index 000000000..8b2286c8f --- /dev/null +++ b/internal/common/types.go @@ -0,0 +1,212 @@ +package common + +import ( + "encoding/json" + "fmt" + "github.com/google/uuid" +) + +// CustomJsonConversionError is thrown when parsing strings into enumerations +type CustomJsonConversionError struct { + reason string +} + +func (err *CustomJsonConversionError) Error() string { + return err.reason +} + +func unmarshalHelper(data []byte, jsonError, typeConversionError string, mapping map[string]int) (int, error) { + var stringInput string + err := json.Unmarshal(data, &stringInput) + if err != nil { + return 0, &CustomJsonConversionError{string(data) + jsonError} + } + value, exists := mapping[stringInput] + if !exists { + return 0, &CustomJsonConversionError{stringInput + typeConversionError} + } + return value, nil +} + +func marshalHelper(input int, mapping map[string]int, errorMessage string) ([]byte, error) { + for k, v := range mapping { + if v == input { + return json.Marshal(k) + } + } + return nil, &CustomJsonConversionError{fmt.Sprintf("%d %s", input, errorMessage)} +} + +// Architecture represents one of the supported CPU architectures available for images +// produced by osbuild-composer. It is represented as an integer because if it +// was a string it would unmarshal from JSON just fine even in case that the architecture +// was unknown. +type Architecture int + +// A list of supported architectures. As the comment above suggests the type system does +// not allow to create a type with a custom set of values, so it is possible to use e.g. +// 56 instead of an architecture, but as opposed to a string it should be obvious that +// hardcoding a number instead of an architecture is just wrong. +// +// NOTE: If you want to add more constants here, don't forget to add a mapping below +const ( + X86_64 Architecture = iota + Aarch64 + Armv7hl + I686 + Ppc64le + S390x +) + +// getArchMapping is a helper function that defines the conversion from JSON string value +// to Architecture. +func getArchMapping() map[string]int { + mapping := map[string]int{ + "x86_64": int(X86_64), + "aarch64": int(Aarch64), + "armv7hl": int(Armv7hl), + "i686": int(I686), + "ppc64le": int(Ppc64le), + "s390x": int(S390x), + } + return mapping +} + +// UnmarshalJSON is a custom unmarshaling function to limit the set of allowed values +// in case the input is JSON. +func (arch Architecture) UnmarshalJSON(data []byte) error { + value, err := unmarshalHelper(data, " is not a valid JSON value", " is not a valid architecture", getArchMapping()) + if err != nil { + return err + } + arch = Architecture(value) + return nil +} + +// MarshalJSON is a custom marshaling function for our custom Architecture type +func (arch Architecture) MarshalJSON() ([]byte, error) { + return marshalHelper(int(arch), getArchMapping(), "is not a valid architecture tag") +} + +type ImageType int + +// NOTE: If you want to add more constants here, don't forget to add a mapping below +const ( + Alibaba ImageType = iota + Azure + Aws + GoogleCloud + HyperV + LiveISO + OpenStack + Qcow2Generic + Vmware +) + +// getArchMapping is a helper function that defines the conversion from JSON string value +// to ImageType. +func getImageTypeMapping() map[string]int { + mapping := map[string]int{ + "Alibaba": int(Alibaba), + "Azure": int(Azure), + "AWS": int(Aws), + "Google Cloud": int(GoogleCloud), + "Hyper-V": int(HyperV), + "LiveISO": int(LiveISO), + "OpenStack": int(OpenStack), + "qcow2": int(Qcow2Generic), + "VMWare": int(Vmware), + } + return mapping +} + +func (imgType ImageType) UnmarshalJSON(data []byte) error { + value, err := unmarshalHelper(data, " is not a valid JSON value", " is not a valid image type", getImageTypeMapping()) + if err != nil { + return err + } + imgType = ImageType(value) + return nil +} + +func (imgType ImageType) MarshalJSON() ([]byte, error) { + return marshalHelper(int(imgType), getImageTypeMapping(), "is not a valid image type tag") +} + +type Distribution int + +// NOTE: If you want to add more constants here, don't forget to add a mapping below +const ( + Fedora30 Distribution = iota + Fedora31 + RHEL82 +) + +// getArchMapping is a helper function that defines the conversion from JSON string value +// to Distribution. +func getDistributionMapping() map[string]int { + mapping := map[string]int{ + "fedora-30": int(Fedora30), + "fedora-31": int(Fedora31), + "rhel-8.2": int(RHEL82), + } + return mapping +} + +func (distro Distribution) UnmarshalJSON(data []byte) error { + value, err := unmarshalHelper(data, " is not a valid JSON value", " is not a valid distribution", getDistributionMapping()) + if err != nil { + return err + } + distro = Distribution(value) + return nil +} + +func (distro Distribution) MarshalJSON() ([]byte, error) { + return marshalHelper(int(distro), getDistributionMapping(), "is not a valid distribution tag") +} + +type UploadTarget int + +// NOTE: If you want to add more constants here, don't forget to add a mapping below +const ( + EC2 UploadTarget = iota + AzureStorage // I mention "storage" explicitly because we might want to support gallery as well +) + +// getArchMapping is a helper function that defines the conversion from JSON string value +// to UploadTarget. +func getUploadTargetMapping() map[string]int { + mapping := map[string]int{ + "AWS EC2": int(EC2), + "Azure storage": int(AzureStorage), + } + return mapping +} + +func (ut UploadTarget) UnmarshalJSON(data []byte) error { + value, err := unmarshalHelper(data, " is not a valid JSON value", " is not a valid upload target", getUploadTargetMapping()) + if err != nil { + return err + } + ut = UploadTarget(value) + return nil +} + +func (ut UploadTarget) MarshalJSON() ([]byte, error) { + return marshalHelper(int(ut), getUploadTargetMapping(), "is not a valid upload target tag") +} + +type ImageRequest struct { + ImgType ImageType `json:"image_type"` + UpTarget []UploadTarget `json:"upload_targets"` +} + +// ComposeRequest is used to submit a new compose to the store +type ComposeRequest struct { + BlueprintName string `json:"blueprint_name"` + ComposeID uuid.UUID `json:"uuid"` + Distro Distribution `json:"distro"` + Arch Architecture `json:"arch"` + RequestedImages []ImageRequest `json:"requested_images"` +} diff --git a/internal/common/types_test.go b/internal/common/types_test.go new file mode 100644 index 000000000..fb9da27ab --- /dev/null +++ b/internal/common/types_test.go @@ -0,0 +1,101 @@ +package common + +import ( + "encoding/json" + "github.com/google/uuid" + "reflect" + "testing" +) + +func TestJSONConversionsComposeRequest(t *testing.T) { + cases := []struct{ + input string + expectedConversionResult ComposeRequest + }{ + // 1st case + { + ` + { + "blueprint_name": "foo", + "uuid": "789b4d42-da1a-49c9-a20c-054da3bb6c82", + "distro": "fedora-31", + "arch": "x86_64", + "requested_images": [ + { + "image_type": "AWS", + "upload_targets": ["AWS EC2"] + } + ] + } + `, + ComposeRequest{ + BlueprintName: "foo", + ComposeID: uuid.UUID{}, + Distro: Fedora31, + Arch: X86_64, + RequestedImages: []ImageRequest{{ + ImgType: Aws, + UpTarget: []UploadTarget{EC2}, + }}, + }, + }, + // 2nd case + { + ` + { + "blueprint_name": "bar", + "uuid": "789b4d42-da1a-49c9-a20c-054da3bb6c82", + "distro": "rhel-8.2", + "arch": "aarch64", + "requested_images": [ + { + "image_type": "Azure", + "upload_targets": ["Azure storage", "AWS EC2"] + } + ] + } + `, + ComposeRequest{ + BlueprintName: "bar", + ComposeID: uuid.UUID{}, + Distro: RHEL82, + Arch: Aarch64, + RequestedImages: []ImageRequest{{ + ImgType: Azure, + UpTarget: []UploadTarget{AzureStorage, EC2}, + }}, + }, + }, + } + + for _, c := range cases { + // Test unmashaling the JSON from the string above + var inputStringAsStruct *ComposeRequest + err := json.Unmarshal([]byte(c.input), &inputStringAsStruct) + if err != nil { + t.Fatal("Failed ot unmarshal ComposeRequest:", err) + } + if reflect.DeepEqual(inputStringAsStruct, c.expectedConversionResult) { + t.Error("Unmarshaled compose request is not the one expected") + } + + // Test marshaling the expected structure into JSON byte array, but since JSON package in golang std lib + // does not have a canonical form (a 3rd party library is necessary) I convert it back to struct and + // compare the resulting structure with the input one + data, err := json.Marshal(c.expectedConversionResult) + if err != nil { + t.Fatal("Failed ot marshal ComposeRequest:", err) + } + var expectedResultAfterMarshaling *ComposeRequest + err = json.Unmarshal(data, &expectedResultAfterMarshaling) + if err != nil { + t.Fatal("Failed ot unmarshal ComposeRequest:", err, ", input:", string(data)) + } + if reflect.DeepEqual(expectedResultAfterMarshaling, c.expectedConversionResult) { + t.Error("Marshaled compose request is not the one expected") + } + } + +} + +