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.

Sign up for GitHub

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

Jump to bottom

[Monitoring] Fix rison error #83987

Merged
merged 2 commits into from
Nov 25, 2020
Merged

[Monitoring] Fix rison error #83987

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
85b2af0
Put the cluster_uuid in quotes
chrisronline Nov 20, 2020
dd24b40
Merge branch 'master' into monitoring/rison_Fix
kibanamachine Nov 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('getSafeForExternalLink', () => {
location
)
).toBe(
`#/overview?_g=(cluster_uuid:NDKg6VXAT6-TaGzEK2Zy7g,filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'))`
`#/overview?_g=(cluster_uuid:'NDKg6VXAT6-TaGzEK2Zy7g',filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'))`
);
});

Expand All @@ -68,7 +68,7 @@ describe('getSafeForExternalLink', () => {
location
)
).toBe(
`#/overview?_g=(filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'),cluster_uuid:NDKg6VXAT6-TaGzEK2Zy7g)`
`#/overview?_g=(filters:!(),refreshInterval:(pause:!t,value:10000),time:(from:'2017-09-07T20:12:04.011Z',to:'2017-09-07T20:18:55.733Z'),cluster_uuid:'NDKg6VXAT6-TaGzEK2Zy7g')`
);
});
});
11 changes: 6 additions & 5 deletions x-pack/plugins/monitoring/public/lib/get_safe_for_external_link.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,16 @@ export function getSafeForExternalLink(

let newGlobalState = globalStateExecResult[1];
Object.keys(globalState).forEach((globalStateKey) => {
let value = globalState[globalStateKey];
if (globalStateKey === 'cluster_uuid') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically rison should already cast strings for us, I don't know why we even have to do this

But if we must, we should first check to see if it's already wrapped in quotes (or, you'll keep adding quotes to the value over and over again). So we should also add the following checks like this:

Suggested change
if (globalStateKey === 'cluster_uuid') {
if (globalStateKey === 'cluster_uuid' && value.charAt() !== '"' && value.charAt() !== '\'') {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where are you seeing the double quotes happening? I'm trying to reproduce it locally but can't seem to find the right UI flow

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is an example: https://demo.elastic.co/app/monitoring#/home

  • Notice how clicking on the first cluster has no quotes ...(cluster_uuid:C7AppFTyQfWdUL3ldutLjQ,
  • And clicking on the second cluster has single quotes ...(cluster_uuid:'0f0rwRiWTjaJPTrORm4PoA',

Since, we don't take "checking first" into account we run the risk of adding unwanted quotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I'm a little confused sorry. Why are you testing this PR on the demo site? Are you connecting your local Kibana instance to that cluster or something?

Locally, with two clusters setup, I get from clicking on one cluster:
https://localhost:5601/app/monitoring#/overview?_g=(ccs:Monitoring,cluster_uuid:'1sO4J1fWQEq8AU628IoBGw',refreshInterval:(pause:!t,value:10000),time:(from:now-1h,to:now))

Then, I see https://localhost:5601/app/monitoring#/overview?_g=(refreshInterval:(pause:!t,value:10000),time:(from:now-1h,to:now),cluster_uuid:'TpRkL2e-QVaJisp084RwzQ') when clicking the other.

I can see in the code how we could be double quoting it, but I can't seem to find the use case of it actually happening (so I can verify the fix works)

Copy link
Contributor

@igoristic igoristic Nov 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, I see it now "globalState" here is not actually the globalState we're use to, more of a way to override parameters (since we only use it in one place).

In which case, why not just pass the following cluster_uuid already as a quoted string here:

kibana/x-pack/plugins/monitoring/public/components/cluster/listing/listing.js

Line 82 in 3f97872

href={getSafeForExternalLink(`#/overview`, { cluster_uuid: cluster.cluster_uuid })}
so it'd look something like:

getSafeForExternalLink(`#/overview`, { cluster_uuid: `'${cluster.cluster_uuid}'` })

This way we don't have to check which key is which and how to cast it.

Might be more of nit/preference on my end, so I'm fine if you decide to keep it. Let me know and I'll approve it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. I worry that requiring the consumer to provide the ' will lead to us forgetting to do that. I'm not sure if there is a better way right now though.

value = `'${value}'`;
}
const keyRegExp = new RegExp(`${globalStateKey}:([^,]+)`);
const execResult = keyRegExp.exec(newGlobalState);
if (execResult && execResult.length) {
newGlobalState = newGlobalState.replace(
execResult[0],
`${globalStateKey}:${globalState[globalStateKey]}`
);
newGlobalState = newGlobalState.replace(execResult[0], `${globalStateKey}:${value}`);
} else {
newGlobalState += `,${globalStateKey}:${globalState[globalStateKey]}`;
newGlobalState += `,${globalStateKey}:${value}`;
}
});

Expand Down