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

Append suffix to param key to uniquify duplicate param keys #232

Merged
merged 2 commits into from
Jan 20, 2022
Merged

Append suffix to param key to uniquify duplicate param keys #232

merged 2 commits into from
Jan 20, 2022

Conversation

rejohnst
Copy link
Contributor

Fixes #169

This PR is my attempt to address issue 169.

Apologies in advance if this proposed fix is too naive - It's been awhile since I've done any JS coding. The fix simply appends a suffix to the param name (key) for each param before adding it to the paramsData map. It then strips the suffix off before constructing the request URL in _request(). I tested this change by loading the plugin in a local grafana instance and modifying an existing dashboard we have for viewing Prometheus alerts - which uses this plugin - to make API calls to Alertmanager. I then repeatedly added/removed various combinations of request parameters (including params with duplicate keys) via the Grafana UI and then verified in each case that the final constructed URL string looked as expected by intercepting the requests using a simple proxy that was placed in front of Alertmamager which would dump the request URL before forwarding the request.

I also ran the unit tests in the repo (output captured below):

$ yarn test
yarn run v1.22.17
$ grafana-toolkit plugin:test
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
Browserslist: caniuse-lite is outdated. Please run:
npx browserslist@latest --update-db

Why you should do it regularly:
https://github.com/browserslist/browserslist#browsers-data-updating
ts-jest[config] (WARN) The option `tsConfig` is deprecated and will be removed in ts-jest 27, use `tsconfig` instead
ts-jest[config] (WARN) The option `tsConfig` is deprecated and will be removed in ts-jest 27, use `tsconfig` instead
ts-jest[config] (WARN) The option `tsConfig` is deprecated and will be removed in ts-jest 27, use `tsconfig` instead
 PASS  src/detectFieldType.test.ts
 PASS  src/parseValues.test.ts
 PASS  src/datasource.test.ts

Test Suites: 3 passed, 3 total
Tests:       20 passed, 20 total
Snapshots:   0 total
Time:        1.752 s, estimated 2 s
Ran all test suites with tests matching "".
✔ Running tests
Done in 2.84s.

@marcusolsson marcusolsson merged commit d694734 into grafana:main Jan 20, 2022
@marcusolsson
Copy link
Contributor

I think the long term fix for this is to not use a map for this, so I'm ok with accepting this for now! Thanks a lot!

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.

Doesn't parse multiple params with the same key
2 participants