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

Changed the processing of Additional JSON #149

Merged

Conversation

istvan86-nagy
Copy link

So it first parses the variables and only after that parses the data as JSON, so variables can also be used as non-string type in the JSON.

Solving #148

…bles and JSON parse after that, so variables can be also used as non-string type in the JSON
@simPod
Copy link
Owner

simPod commented Feb 22, 2021

I'll try to run this in my project ASAP (tmrw I hope). Thanks for patience!

@simPod
Copy link
Owner

simPod commented Feb 24, 2021

I tried with string-type variable as

{ "IncludeFilteredOutItems": $stringVar }

which produces

{ "IncludeFilteredOutItems": stringvalue }

resulting in incorrect json

@istvan86-nagy
Copy link
Author

istvan86-nagy commented Feb 24, 2021

string variables still need to be added as strings, so I think that shouldn't be a problem.

So for non-string types you can use:

{ "IncludeFilteredOutItems": $nonStringVar }

and for string types you can use:

{ "IncludeFilteredOutItems": "$stringVar" }

@simPod
Copy link
Owner

simPod commented Feb 24, 2021

I'd prefer universal solution so you don't need to take into account what type of value is in variable. Also the type of the value might be dynamic, eg. when loading value from another data source. Not sure if it's doable though.

I guess we might start by parametrising the test allowing us to test multiple type variations easily. Definitely need tests for

  • boolean ☑
  • number ☑
  • string
  • null
  • lists of all above
  • did I forget anything?

@istvan86-nagy
Copy link
Author

I did some testing based on your feedback and found the following:

By having a universal solution:

  1. For simple cases when you only have your variable as a value, with a small change in the code, it would replace this out of the box:
    { "IncludeFilteredOutItems": $StringVar }
    =>
    { "IncludeFilteredOutItems": "StringValue" }
  2. However in a more complex case (there is even a test case for that), we cannot do that, we have to apply extra quotes
    • Without the extra quotes:

      { "IncludeFilteredOutItems": $StringVar ms }

      =>

      { "IncludeFilteredOutItems": "StringValue" ms } // Invalid JSON
    • With extra quotes:

      { "IncludeFilteredOutItems": "$StringVar ms" }

      =>

      { "IncludeFilteredOutItems": ""StringValue" ms" } // Invalid JSON

@istvan86-nagy
Copy link
Author

I personally think that giving the control to the users isn't necessarily a bad thing. Users should know what type of their variables are and if not, they could always just use them as string:

So

  1. for string and unknown types you can use
    { "IncludeFilteredOutItems": "$StringOrUnknownVar" }
  2. other types:
    { "IncludeFilteredOutItems": $Non-StringOrUnknownVar }

In my opinion it would also be more clear/transparent how the variable substitution works.

@simPod
Copy link
Owner

simPod commented Feb 27, 2021

Hm, the issue seems more complex. See https://grafana.com/docs/grafana/latest/variables/syntax/#variable-syntax where they say

$varname This syntax is easy to read, but it does not allow you to use a variable in the middle of a word. Example: apps.frontend.$server.requests.count

So for example we should not consider supporting { "IncludeFilteredOutItems": $Non-StringOrUnknownVar } at all. We have to try to comply with Grafana standards as much as possible.

Also, if you check their docs ${var_name} and $varname are always interpolated as strings.

So for your use case, I'd first try to discuss with Grafana if it makes sense to implement something like ${var:number} on their side. (https://grafana.com/docs/grafana/latest/variables/advanced-variable-format-options/)

@istvan86-nagy
Copy link
Author

istvan86-nagy commented Feb 27, 2021

I think there might be some misinterpretation here. The doc says indeed that $VarName cannot be used in middle of an expression so the next point refers to what should be used insted (${VarName}):

  • $varname This syntax is easy to read, but it does not allow you to use a variable in the middle of a word. Example: apps.frontend.$server.requests.count
  • ${var_name} Use this syntax when you want to interpolate a variable in the middle of an expression.

I didn't find any reference for that either:

Also, if you check their docs ${var_name} and $varname are always interpolated as strings.

Also at this moment their variable interpolation works with both string, number, boolean, etc out of the box, the only thing prevents it from working is the code in this repo parsing the data input into JSON before doing the variable interpolation.

So for your use case, I'd first try to discuss with Grafana if it makes sense to implement something like ${var:number} on their side.

Even if that change would be made (while there is no need for it), the plugin code still wouldn't make it possible to use it at this moment since the JSON parsing would still fail due to the current code setup.

So if the built in variable interpolation supports using non string variables, I don't see why this change wouldn't comply with Grafana standards.

@istvan86-nagy
Copy link
Author

istvan86-nagy commented Feb 27, 2021

So the issue is that piece of code:

if (target.data.trim() !== '') {
target.data = JSON.parse(target.data, (key, value) => {
if (typeof value === 'string') {
return value.replace((getTemplateSrv() as any).regex, (match) => this.cleanMatch(match, options));
}
return value;
});
}

  1. We trim and check if Additional JSON is empty
  2. We parse the Additional JSON input to JSON
  3. We would do the variable interpolation, but with
    { "Foo": $Bar }
    we don't even reach that step, because the code fails on JSON parsing on step nr 2 (line 163). So that's why I wonder how does that have anything to do with complying to Grafana standards, while we haven't even reached the Grafana variable interpolation part when the code fails already.

While with the change proposed:

  1. We trim and check if Additional JSON is empty
  2. Do the variable interpolation
  3. Parse the interpolated input to JSON

@simPod
Copy link
Owner

simPod commented Feb 27, 2021

Aight, let's ship it. It does not break any tests (I'll have to look into them anyway, it's kind of mess in my opinion :) ) and you have a point. This is step forward.

Thank you!

🚢

@istvan86-nagy
Copy link
Author

Thank you too!

@simPod
Copy link
Owner

simPod commented Mar 4, 2021

I'll put this in my production by the end of this week and next week I'd release it.

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.

3 participants