BED-31: Upgrade axios from 0.17.1 to latest stable - #117
Conversation
|
@jayasanka-sack hi jay ,can I get a review on this please |
| }); | ||
|
|
||
| const error = errorResponse.response.data ? errorResponse.response.data.error : errorResponse; | ||
| const error = errorResponse.response && errorResponse.response.data ? errorResponse.response.data.error : errorResponse; |
There was a problem hiding this comment.
Since this catch block is exactly what the PR is hardening, a one-word fix worth folding in while you're here: the this.setState at the top of this handler (line 120) throws before this line ever runs. The callback is a plain function, so this is undefined in the transpiled strict-mode code. Net effect: when a save fails, the user gets no error toast and the form is left stuck with disableSubmit: true. Changing it to self.setState, as every other handler in this sweep already does, makes the guard you added here actually reachable. It predates your change and this is the only file with the problem (I checked the others), so I'd also be fine with it as a quick follow-up if you'd rather keep this PR mechanical.
| import AdmissionLocationWrapper from "components/admissionLocation/admissionLocationWrapper.js"; | ||
| import MockAdapter from "axios-mock-adapter"; | ||
| import axios from "axios/index"; | ||
| import axios from "axios"; |
There was a problem hiding this comment.
Optional, and I wouldn't hold the PR for it: none of the suites exercise an error path, so the handler guards (the main behavioral fix here) have no test that fails on 0.17.1 and passes after the upgrade. axios-mock-adapter resolves to 1.22.0 in the lockfile, which supports .networkError() for exactly this failure shape. One case along the lines of
mock.onGet('https://192.168.33.10/openmrs/ws/rest/v1/bedtype').networkError();plus an assertion that the wrapper notifies instead of throwing would pin the new behavior down, so a future change can't quietly reintroduce the crash.
|
|
Hey @dkayiwa Thanks so much for the detailed review and for catching that this.setState bug! I completely agree that it makes sense to fix it while we're in here hardening the error handling. I've updated the catch block in addEditAdmissionLocation.js to correctly use self.setState, so if a request fails, the component will actually recover and display the error toast as intended rather than silently crashing. I double-checked the file and this was indeed the only spot missing the const self = this; pattern. |
|
I also went ahead and took your suggestion to add a test case! I added a new test in admissionLocationWrapper-test.js using mock.onGet(...).networkError(). I verified that it properly fails if the this.setState bug is present, and now passes flawlessly with the fix, proving that the notify() behavior is successfully pinned down against any future regressions. |
2 participants
Requirements
Summary
This PR upgrades the axios dependency in the bedmanagement-owa module from 0.17.1 to ^1.15.0. The older version carried 20 known security vulnerabilities, including high-severity issues like SSRF, ReDoS, and credential leaks.
Key improvements and fixes:
Bumps axios to ^1.15.0 to resolve all security vulnerabilities.
Fixes brittle .catch() error handlers across 13 components. Axios 1.x exposes a bug in the old implementation where network errors (where .response is undefined) would throw a fatal TypeError. Handlers now safely verify .response exists before attempting to access .data.
Updates webpack.config.js to alias axios to its pre-transpiled browser distribution (axios/dist/axios.js). This resolves build failures caused by the legacy Webpack 3 compiler choking on Axios 1.x's modern ES6 syntax.
Fixes broken Jest imports in admissionLocationWrapper-test.js to use import axios from "axios" rather than axios/index, which was removed in 1.x.
Related Issue
https://openmrs.atlassian.net/browse/BED-31
Other
Validated with yarn audit which confirms zero vulnerabilities remaining in the Axios dependency tree.
Verified locally that all 56 frontend Jest tests and 46 backend Maven tests pass successfully without any regressions.