From ec6fd962f5c8a8758ddb1873ee343483be3eb9bf Mon Sep 17 00:00:00 2001 From: Aidan Oldershaw Date: Wed, 7 Jul 2021 16:11:26 -0400 Subject: [PATCH] B: unmarshal using json.Number rather than float64 by default, "encoding/json" (and hence "sigs.k8s.io/yaml") decode any number as float64 (if you try to decode into a generic type like interface{}) vars/template.go does not allow interpolating float64 into a string, presumably because you could end up interpolating the number 123.0 as e.g. 122.99999999999999 due to limitations with float precision. this means that if you ever load_var on a file containing a number (even an integer), then you can never interpolate that number into a string, which can be pretty annoying. json.Number, on the other hand, stores the number as a string, so there is no issue with precision - and it means ints will stay as "ints". since we never use these values directly (they are just serialized/deserialized), this *shouldn't* cause issues. Signed-off-by: Aidan Oldershaw --- atc/creds/eval.go | 7 ++++++- atc/exec/load_var_step.go | 18 ++++++++++++---- atc/exec/load_var_step_test.go | 38 ++++++++++++++++++---------------- testflight/across_test.go | 2 ++ testflight/fixtures/across.yml | 2 +- vars/template.go | 4 ++++ 6 files changed, 47 insertions(+), 24 deletions(-) diff --git a/atc/creds/eval.go b/atc/creds/eval.go index c96d159e2..39d4e4417 100644 --- a/atc/creds/eval.go +++ b/atc/creds/eval.go @@ -22,5 +22,10 @@ func evaluate(variablesResolver vars.Variables, in, out interface{}) error { return err } - return yaml.Unmarshal(bytes, out) + return yaml.Unmarshal(bytes, out, useJSONNumber) +} + +func useJSONNumber(decoder *json.Decoder) *json.Decoder { + decoder.UseNumber() + return decoder } diff --git a/atc/exec/load_var_step.go b/atc/exec/load_var_step.go index 3e01c2417..1e3f227ac 100644 --- a/atc/exec/load_var_step.go +++ b/atc/exec/load_var_step.go @@ -1,8 +1,10 @@ package exec import ( + "bytes" "context" "encoding/json" + "errors" "fmt" "io/ioutil" "path/filepath" @@ -157,14 +159,17 @@ func (step *LoadVarStep) fetchVars( var value interface{} switch format { case "json": - value = map[string]interface{}{} - err = json.Unmarshal(fileContent, &value) + decoder := json.NewDecoder(bytes.NewReader(fileContent)) + decoder.UseNumber() + err = decoder.Decode(&value) if err != nil { return nil, InvalidLocalVarFile{file, "json", err} } + if decoder.More() { + return nil, InvalidLocalVarFile{file, "json", errors.New("invalid json: characters found after top-level value")} + } case "yml", "yaml": - value = map[string]interface{}{} - err = yaml.Unmarshal(fileContent, &value) + err = yaml.Unmarshal(fileContent, &value, useJSONNumber) if err != nil { return nil, InvalidLocalVarFile{file, "yaml", err} } @@ -179,6 +184,11 @@ func (step *LoadVarStep) fetchVars( return value, nil } +func useJSONNumber(decoder *json.Decoder) *json.Decoder { + decoder.UseNumber() + return decoder +} + func (step *LoadVarStep) fileFormat(file string) (string, error) { if step.isValidFormat(step.plan.Format) { return step.plan.Format, nil diff --git a/atc/exec/load_var_step_test.go b/atc/exec/load_var_step_test.go index 3b10bbb8b..c19d721de 100644 --- a/atc/exec/load_var_step_test.go +++ b/atc/exec/load_var_step_test.go @@ -2,6 +2,7 @@ package exec_test import ( "context" + "encoding/json" "strings" "code.cloudfoundry.org/lager/lagerctx" @@ -25,11 +26,12 @@ const plainString = " pv \n\n" const yamlString = ` k1: yv1 k2: yv2 +k3: 123 ` const jsonString = ` { - "k1": "jv1", "k2": "jv2" + "k1": "jv1", "k2": "jv2", "k3": 123 } ` @@ -159,7 +161,7 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { + It("var should be parsed correctly", func() { expectLocalVarAdded("some-var", strings.TrimSpace(plainString), true) }) }) @@ -180,7 +182,7 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { + It("var should be parsed correctly", func() { expectLocalVarAdded("some-var", plainString, true) }) }) @@ -201,8 +203,8 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "jv1", "k2": "jv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "jv1", "k2": "jv2", "k3": json.Number("123")}, true) }) }) @@ -222,8 +224,8 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2", "k3": json.Number("123")}, true) }) }) @@ -243,8 +245,8 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2", "k3": json.Number("123")}, true) }) }) }) @@ -265,7 +267,7 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly as trim", func() { + It("var should be parsed correctly as trim", func() { expectLocalVarAdded("some-var", strings.TrimSpace(plainString), true) }) }) @@ -285,8 +287,8 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "jv1", "k2": "jv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "jv1", "k2": "jv2", "k3": json.Number("123")}, true) }) }) @@ -305,8 +307,8 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2", "k3": json.Number("123")}, true) }) }) @@ -325,21 +327,21 @@ var _ = Describe("LoadVarStep", func() { Expect(stepOk).To(BeTrue()) }) - It("should var parsed correctly", func() { - expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2"}, true) + It("var should be parsed correctly", func() { + expectLocalVarAdded("some-var", map[string]interface{}{"k1": "yv1", "k2": "yv2", "k3": json.Number("123")}, true) }) }) }) Context("when file is bad", func() { - Context("when json file is bad", func() { + Context("when json file is invalid JSON", func() { BeforeEach(func() { loadVarPlan = &atc.LoadVarPlan{ Name: "some-var", File: "some-resource/a.json", } - fakeArtifactStreamer.StreamFileFromArtifactReturns(&fakeReadCloser{str: plainString}, nil) + fakeArtifactStreamer.StreamFileFromArtifactReturns(&fakeReadCloser{str: jsonString + "{}"}, nil) }) It("step should fail", func() { diff --git a/testflight/across_test.go b/testflight/across_test.go index aca385f1b..bf111e8cf 100644 --- a/testflight/across_test.go +++ b/testflight/across_test.go @@ -17,8 +17,10 @@ var _ = Describe("A pipeline containing an across step", func() { Expect(watch).To(gbytes.Say("running across static1 dynamic1")) Expect(watch).To(gbytes.Say("running across static1 dynamic2")) Expect(watch).To(gbytes.Say("running across static1 dynamic3")) + Expect(watch).To(gbytes.Say("running across static1 4")) Expect(watch).To(gbytes.Say("running across static2 dynamic1")) Expect(watch).To(gbytes.Say("running across static2 dynamic2")) Expect(watch).To(gbytes.Say("running across static2 dynamic3")) + Expect(watch).To(gbytes.Say("running across static2 4")) }) }) diff --git a/testflight/fixtures/across.yml b/testflight/fixtures/across.yml index 9dd4db469..16b74d994 100644 --- a/testflight/fixtures/across.yml +++ b/testflight/fixtures/across.yml @@ -5,7 +5,7 @@ resources: source: create_files: data.yml: | - [dynamic1, dynamic2, dynamic3] + [dynamic1, dynamic2, dynamic3, 4] jobs: - name: job diff --git a/vars/template.go b/vars/template.go index f36a5f197..05eb08c73 100644 --- a/vars/template.go +++ b/vars/template.go @@ -31,6 +31,10 @@ func (t Template) ExtraVarNames() []string { func (t Template) Evaluate(vars Variables, opts EvaluateOpts) ([]byte, error) { var obj interface{} + // Note: if we do end up changing from "gopkg.in/yaml.v2" to + // "sigs.k8s.io/yaml" here, we'll want to ensure we call + // `json.Decoder.UseNumber()` so that we don't lose precision unmarshaling + // numbers to float64. err := yaml.Unmarshal(t.bytes, &obj) if err != nil { return []byte{}, err