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

Dashboards: Skip inherited object variable names #79567

Merged
merged 3 commits into from
Dec 19, 2023

Conversation

jarben
Copy link
Contributor

@jarben jarben commented Dec 15, 2023

What is this feature?

This PR fixes an issue where variables containing inherited object property names fail with TypeError: T[Q].add is not a function here. This issue has been reported in this escalation.

This is because if (varName! in panelsByVar) { returns true for any variable name that matches inherited prop names.

We can potentially use if (panelsByVar.hasOwnProperty(varName)); however, this would add variables such as toString and would fail anyway as reported here and [here](https://github.com/grafana/grafana/issues/67342).

Which issue(s) does this PR fix?:
Fixes https://github.com/grafana/support-escalations/issues/8743

Also relates to: https://community.grafana.com/t/error-updating-options-t-e-add-is-not-a-function/71728/7

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@jarben jarben requested a review from a team as a code owner December 15, 2023 10:41
@jarben jarben requested review from dprokop and kaydelaney and removed request for a team December 15, 2023 10:41
@grafana-delivery-bot grafana-delivery-bot bot added this to the 10.3.x milestone Dec 15, 2023
@jarben jarben changed the title Skip inherited object variable names Dashboards: Skip inherited object variable names Dec 15, 2023
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @jarben 😄 , great attempt to fix this issue. Because our variable system is complex, I would like to gain more context of the original escalation, would you mind sharing a dashboard example where the escalation issue can be reproduced?

So far, I tested your branch locally, taking the use case from the other related issue: #79212 (using $toString in a query variable), and I still see the problem.

toStringStillTriggersJsError.mp4

I believe the reason is the getVariableName from inspect/utils is not the same function we are using when we are interpolating variables(getTemplateSrv().replace).

@jarben
Copy link
Contributor Author

jarben commented Dec 18, 2023

Hi @Alexa Vargas, thanks for looking at this! The issue can be reproduced here. You can add yourself to the org by adding your email to staff membership here.

@jarben
Copy link
Contributor Author

jarben commented Dec 18, 2023

I believe the reason is the getVariableName from inspect/utils is not the same function we are using when we are interpolating variables(getTemplateSrv().replace).

Good point @axelavargas , this would prevent the error to appear:
image

Although I wonder if we want the variable to not show at all here?
image

Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @jarben 😄 , Thanks for granting me access to the original issue.

I have tested on my local machine a dashboard with Mongo DB and variables and works as expected, the errors disappeared 🥳 .

VariablesMongoWorkingAgain.mp4

Also Issues 67342 and 79212 are still unresolved, and I am afraid the change you suggested in the getVariabledAtIndex will not work as should be. We will prevent showing the error, but the expected behavior should be to do the query by taking the $toString as a normal string. After researching, I found we don't support escaping variable expressions in the query, check this issue #40603 .

I think your PR will only fix the issue from the escalation 🙌🏾 . However, we will need to tackle the other ones in another PR (A bit challenging to be honest with the current variable system)

@jarben jarben merged commit 315dd75 into main Dec 19, 2023
16 checks passed
@jarben jarben deleted the jarben/skip-inherited-object-variable-names branch December 19, 2023 14:08
@summerwollin summerwollin modified the milestones: 10.3.x, 10.3.0 Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants