Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add fromJson and mustFromJson to values func map #164

Merged
merged 4 commits into from
Aug 25, 2020
Merged

Conversation

wI2L
Copy link
Contributor

@wI2L wI2L commented Jul 11, 2020

  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature: This adds two new functions fromJson and mustFromJson to the func map of the Values object.

  • What is the current behavior? (You can also link to an open issue here)

Currently, one can use the toJson function of the Sprig project to encode a value to JSON, but the inverse is not available.

  • What is the new behavior (if this is a feature change)?

A JSON document can be decoded into a data structure, e.g {{eval `varReturningJSON` | fromJson}}, or {{fromJson `{"a":"b"}`}}.

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

New feature, no breaking changes.

  • Other information:

Similar feature request is still on hold for Sprig: Masterminds/sprig#230

@ovh-cds
Copy link
Collaborator

ovh-cds commented Jul 11, 2020

CDS Report test#763.0 ✘

  • Stage 1
    • Test ✘

}

// mustFromJSON decodes JSON into a structured value, returning errors.
func (v *Values) mustFromJSON(s string) (interface{}, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning a (possibly zero value) reflect.Value instead of interface?
See field implementation, it gets properly considered as a zero value for eg coalesce, default, '' replacement by the text tempmate engine, etc

Copy link
Contributor Author

@wI2L wI2L Jul 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. What should we consider as "zero-value" in the context of a JSON-value tho ?
reflect.ValueOf(nil) will return the zero Value, which is great, but what about empty string, zero numbers, and so on ?

README.md Outdated
| **`field`** | Equivalent to the dot notation, for entries with forbidden characters | ``{{field `config` `foo.bar`}}`` |
| **`eval`** | Evaluates the value of a template variable | ``{{eval `var1`}}`` |
| **`evalCache`** | Evaluates the value of a template variable, and cache for future usage (to avoid further computation) | ``{{evalCache `var1`}}`` |
| **`fromJson`** | Decodes a JSON document into a structure. If the input cannot be decoded as JSON, the function will return an empty string | ``{{fromJson `{"a":"b"}`}}`` |
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be interesting to describe a common use case for this here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I guess, the most common use case we could document is to return a JSON stringified data structure from a JavaScript expression (object, array), and use one of its members in the template.
Example: {{(eval `myExpression` | fromJson).myArr}} or {{(eval `myExpression` | fromJson).myObj}}.

@wI2L
Copy link
Contributor Author

wI2L commented Jul 11, 2020

One issue I couldn't figure how to circumvent yet, is the fact that a value extracted from a data-structure decoded with the introduced functions will always be converted to the Golang textual representation of their type.

See the following test case TestJSONParsing:

  assert.Equal(t, "utask", output["a"])
  assert.Equal(t, "666", output["b"])
  assert.Equal(t, "map[k:v]", output["c"])
  assert.Equal(t, "[1 2 3]", output["d"])

We are bound by the YAML spec, since we MUST quote our blocks in the template:

  a: '{{(eval `rawJSON` | fromJson).a}}'
  b: '{{(eval `rawJSON` | mustFromJson).b}}'
  c: '{{(eval `rawJSON` | mustFromJson).c}}'
  d: '{{(eval `rawJSON` | fromJson).d}}'

The idea is to be able to pass a map or slice as the input of a subtask without having the value converted to a string.

  variables:
    - name: myVariable
      expression: >
        var o = ["1", "2", "3"];
        JSON.stringify(o);

  myStep:
    action:
      type: subtask
      configuration:
        template: my-template
        input:
          foobar: '{{eval `myVariable` | fromJson}}' # <-- will pass '[1 2 3]'

The field input is a map[string]interface{}, how do we pass a Go slice without the string conversion when the myVariable expression evaluation returns a JSON array ?

@wI2L
Copy link
Contributor Author

wI2L commented Aug 20, 2020

I think we can't do more about about the issue I raised in my previous comment #164 (comment). The implementation is good enough for now, we can still leverage the new fromJSON functions for useful cases.

@rbeuque74 A second review would be nice, I'll address the minor changes suggested by @loopfz.

Signed-off-by: William Poussier <william.poussier@gmail.com>
Signed-off-by: William Poussier <william.poussier@gmail.com>
Signed-off-by: William Poussier <william.poussier@gmail.com>
Signed-off-by: William Poussier <william.poussier@gmail.com>
@loopfz loopfz merged commit efe2182 into master Aug 25, 2020
@rbeuque74 rbeuque74 deleted the fromJSON-func branch August 25, 2020 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants