bug: @expr with template forcibly converts string value to primitive value about tailcall

[bnchi]

@meskill

Seems like mustache is parsing these values correctly the root cause if I'm not mistaken is when render_value is getting invoked and reaching this line :

tailcall/src/core/serde_value_ext.rs

Line 20 in 9cdf3ef

serde_json::from_str::<GraphQLValue>(rendered.as_ref())

[bnchi]

This bug is trickier than I thought. The line I shared above doesn't seem to be the main root cause, if I find s solid fix I'll push a PR.

[meskill]

the mustache parser probably not the issue itself (although, it does strip " symbols from input on parsing) but mostly the render. Since Mustache::render is mostly made for using in strings like urls the output is generated without quotes and when we try to convert it back to serde::Value we get the converted value that could be a string in the first place.

I think it could be solved with the change like was made here https://github.com/tailcallhq/tailcall/pull/1556/files#diff-855a66b57f6600cfd1af0da76b2be55e275d89f3d3fb57cfa03255b1b0aef453 i.e. return Value from Mustache::render instead of string. But this pr wasn't merged due to performance implications.

[bnchi]

the mustache parser probably not the issue itself (although, it does strip " symbols from input on parsing) but mostly the render. Since Mustache::render is mostly made for using in strings like urls the output is generated without quotes and when we try to convert it back to serde::Value we get the converted value that could be a string in the first place.

I think it could be solved with the change like was made here https://github.com/tailcallhq/tailcall/pull/1556/files#diff-855a66b57f6600cfd1af0da76b2be55e275d89f3d3fb57cfa03255b1b0aef453 i.e. return Value from Mustache::render instead of string. But this pr wasn't merged due to performance implications.

That's what I concluded too and why I said it's tricky for me because I don't know the codebase well.
I was thinking about changing the PathString trait and making the path_string return a tuple something like (type, Cow) where the type is the type that got converted and we can match against it inside that block if it was a string simply return GraphQLValue::String otherwise keep the logic as is, but I don't know if that's a solid idea because it require all the implementers of this trait to do the same thing. what are you thoughts ?

tailcall/src/core/path.rs

Line 17 in 5f54bca

fn path_string<T: AsRef<str>>(&self, path: &[T]) -> Option<Cow<'_, str>>;

[meskill]

seems like the issue could be resolved by this issue #1535 and more precisely this part "Optimize serde_value_ext::render_value. It renders itself as a string and then deserializes it as json."

@tusharmath do you have any plans on #1535?

[bnchi]

Just wanted to extend on my idea to make sure it's clear enough, I re-read the the implementers of PathString and if I would to return a tuple from the path_string the type that's going to be returned is simply an enum that looks exactly the same as serde_json::Value and async_graphql::Value

Something like :

enum TailCallValue {
 String,
 List,
 Object,
 Bool
}

it's a cheap one but it could solve this problem(with potentially not a lot of changes)

[github-actions]

Action required: Issue inactive for 30 days.
Status update or closure in 7 days.

[github-actions]

Issue closed after 7 days of inactivity.