fix(templating)!: return json, not go structs

[b-erdem]

Motivation

When you extract a value from json using jsonpath expressions it returns values properly, only for simple values. For instance when you pass the following JSON:
{"employees": [{"name": "Baris", "age":43},{"name": "Mo", "age": 42}, {"name": "Jai", "age" :44}]} and $.employees[0].name it'll return "Baris" correctly. But let's say if you want to return an item in the array, for instance with $.employees[0] query then you'll get map[age:43 name:Baris]. This is not correct, it returns internal representation of the value when returned value is not a simple value.

We're passing some complex values to each steps and some of the values returned from a jsonpath expression could be a complex object. So we face with issues and doing some workarounds to get this correctly. Instead I thought maybe it'd be useful to fix it directly here. With this PR we'll be able to get values correctly from jsonpath expressions. For instance $.employees[0] now would return: {"age":43,"name":"Baris"}

Modifications

The returned value is now passed to json.Marshal directly without formatting it. fmt.Sprintf("%v", result)) this actually causes getting internal representation of the value.

The rest of the code is making sure we're returning a correct json value/object, mainly related to quotes. The changes I did in this PR is to cover when the returned object is a complex object and it contains strings, so we'll have quotes inside of the value. Then we need to properly encode them.

Verification

• Added some unit tests.
• Added an example with a complex return value.
• Tested locally with some of our workflows.

Please let me know if there's anything missing or something I can improve more. Thanks!

[Joibel]

LGTM.

Thanks @b-erdem for your contribution.

[b-erdem]

Hello @agilgur5 I think I need another approval to get this merged. :)

[Joibel]

This is a breaking change from previous bad but usable behaviour. I am not sure whether this should be held until 3.6. If it goes in 3.5.6 it should be highlighted in the notes.

[agilgur5]

@Joibel but did the previous behavior ever make sense / was it ever useful? From a quick glance, it sounds like it would always cause an error, so then we could straightforwardly merge this as a patch and wouldn't even necessarily need notes as the previous behavior did not work.

I feel like there might have been a use for this behavior before but I couldn't think of one off the top of my head. Using deep, nested structures without a pre-parsing step was not a use-case I had used much though.

[Joibel]

The previous behaviour didn't result in an error, was deterministic and predictable (you got the go printf %v representation).

This could have been used, it contains the values you want, just not in the representation expected. I wouldn't be surprised if someone somewhere does rely on this, and so we should point out that we are changing it.

[b-erdem]

@Joibel @agilgur5 Hello! Do we have a plan when we want to merge this? 🤔

[b-erdem]

The previous behaviour didn't result in an error, was deterministic and predictable (you got the go printf %v representation).

This could have been used, it contains the values you want, just not in the representation expected. I wouldn't be surprised if someone somewhere does rely on this, and so we should point out that we are changing it.

Hi! As far as I tested, nothing changes for simple values. For complex values, since it returns go struct, it's not usable when you pass to the next steps(I'm not sure if there are cases where it works fine, though).

Even if it causes some issues for some people, it looks like an easy change to do. 🤔

Lastly, do you have an idea to make sure we don't break anything? cc: @agilgur5

[Joibel]

I am happy to merge this and put something into the release notes. It was wrong, but didn't error and was usable, so wanted to call that out.

[b-erdem]

I am happy to merge this and put something into the release notes. It was wrong, but didn't error and was usable, so wanted to call that out.

I also think that way. Thanks! Who should I ask for additional approval that gives me the right to merge this PR then? 🤔

[Joibel]

I'll merge it now, the checks weren't green, but they are now.

[agilgur5]

Just re-stumbled upon #8930, which includes workarounds users have been using for some time that we would not want to break.
In general, expressions on structs should continue to work -- I'm not sure if this would break them or not.
We should add those to the test cases

Also if it does, we should really add the conventional ! for a breaking change and only release this in a minor, not a patch

[Joibel]

Notes for maintainers: this will come out in 3.6 and should be noted in the release notes. Please don't backport to any other branches.

[agilgur5]

Mmm if this is going to be a breaking change, the release note should really have been included in this PR too -- we already have some for 3.6 on main (e.g. #12653).
Also test cases would still be useful, as if there is some breakage for those cases, we can explicitly note them.

@Joibel @b-erdem can either of you work on that?

[b-erdem]

@agilgur5 Hi, I’ll look into this next week.

[agilgur5]

@Joibel @isubasinghe could one of you take over the release notes and test cases for this?
Since @b-erdem has not added that and it is pretty critical to do prior to a release as well as before we forget about this

The cases I mentioned above of expressions on structs such as in #8930 and #12739 in particular need test cases and need checking for breakage.

[agilgur5]

I wrote up a follow-up issue to track the above in #13541