feat: query string dynamic app config #3391

anarnoli · 2023-05-14T06:09:18Z

Context

This PR allows for the app config to reference a json over network. This allows for
fast swapping of data source configuration, OIDC, etc, without recompiling the application.

If the configUrl query string is passed, the worklist and modes will load from the referenced json
rather than the default .env config

If there is no configUrl path provided, the default behaviour is used and there should not be any
deviation from current user experience.

Points to consider

User have to enable this feature by setting dangerouslyUseDynamicConfig.enabled:true. By default it is false.
Regex helps to avoid easy exploit
User must set cross-origin = same-origin to avoid potential harder exploit

Sample local config

/* Dynamic config allows user to pass "configUrl" query string this allows to load config without recompiling application. The regex will ensure valid configuration source */
  dangerouslyUseDynamicConfig: {
    enabled: false,
    regex: /.*/,
  }

Example usage

http://localhost:3000/?configUrl=http://localhost:3000/config/example.json

This PR is related to another PR #2925

Checklist

PR

Brief description of changes
Links to any relevant issues
Required status checks are passing
User cases if changes impact the user's experience
@mention a maintainer to request a review

Code

My code has been well-documented (function documentation, inline comments,
etc.)

Public Documentation Updates

The documentation page has been updated as necessary for any public API
additions or removals.

netlify · 2023-05-14T06:09:27Z

✅ Deploy Preview for ohif-platform-docs ready!

Name	Link
🔨 Latest commit	`3281fdc`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-docs/deploys/64627bf87a3ac100088ef16b
😎 Deploy Preview	https://deploy-preview-3391--ohif-platform-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify · 2023-05-14T06:09:28Z

✅ Deploy Preview for ohif-platform-viewer ready!

Name	Link
🔨 Latest commit	`3281fdc`
🔍 Latest deploy log	https://app.netlify.com/sites/ohif-platform-viewer/deploys/64627bf88b02de00084efa45
😎 Deploy Preview	https://deploy-preview-3391--ohif-platform-viewer.netlify.app/
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2023-05-14T06:14:09Z

Codecov Report

Merging #3391 (3281fdc) into v3-stable (9f1e813) will increase coverage by 13.20%.
The diff coverage is n/a.

@@              Coverage Diff               @@
##           v3-stable    #3391       +/-   ##
==============================================
+ Coverage      25.15%   38.36%   +13.20%     
==============================================
  Files            119       82       -37     
  Lines           2862     1345     -1517     
  Branches         555      303      -252     
==============================================
- Hits             720      516      -204     
+ Misses          1856      663     -1193     
+ Partials         286      166      -120

see 77 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4172d04...3281fdc. Read the comment docs.

anarnoli · 2023-05-14T06:15:09Z

@sedghi @wayfarer3130 - request to review this PR.

@cupidchan FYI.

wayfarer3130 · 2023-05-15T12:29:13Z

extensions/default/src/ViewerLayout/index.tsx

+    const query = new URLSearchParams(window.location.search);
+    const configUrl = query.get('configUrl');
+
+    const searchQuery = new URLSearchParams();


Can you just add a TODO here to handle the remembering of the parameters in a better way? Really we should add the relevant parameters to the other queries, and then restore the full query string on going back, but that is NOT your problem here, it is just that I want a reminder that having the viewer page know about hte query page isn't a good idea.

wayfarer3130 · 2023-05-15T12:30:41Z

platform/viewer/public/config/default.js

@@ -25,6 +25,16 @@ window.config = {
  },
  // filterQueryParam: false,
  defaultDataSourceName: 'dicomweb',
+  /* Dynamic config allows user to pass "configUrl" query string this allows to load config without recompiling application. The regex will ensure valid configuration source */
+  dangerouslyUseDynamicConfig: {


You can leave the configuration commented out with the enabled: true set to show how to enable it. That way the entire block just works if someone uncomments it.

I agree @wayfarer3130 .

wayfarer3130 · 2023-05-15T12:32:13Z

platform/viewer/src/loadDynamicConfig.js

+  const useDynamicConfig = config.dangerouslyUseDynamicConfig;
+
+  // Check if dangerouslyUseDynamicConfig enabled
+  if (useDynamicConfig.enabled) {


This has to be useDynamicConfig?.enabled so that if the value isn't configured, it won't throw.

wayfarer3130 · 2023-05-15T12:34:01Z

platform/viewer/src/loadDynamicConfig.js

+      // validate regex
+      const regex = useDynamicConfig.regex;
+
+      if (regex.test(configUrl)) {


Use:
configUrl.match(regex) so that if a string is provided int he configuration, it doesn't fail to work.

wayfarer3130 · 2023-05-15T12:40:40Z

platform/viewer/src/routes/WorkList/WorkList.tsx

            return (
              <Link
                key={i}
                to={`${dataPath ? '../../' : ''}${mode.routeName}${dataPath ||
-                  ''}?StudyInstanceUIDs=${studyInstanceUid}`}
+                  ''}?${decodeURIComponent(query.toString())}`}


This fails for the situation where the configUrl includes parameters, such as:
configUrl=https://myhost/config?a=b&c=d
because the & is encoded/decoded. The decodeURIComponent, as the name says, needs to apply to individual components.

@wayfarer3130 Do you mean decodeURIComponent should be removed and original queryString should be passed?

wayfarer3130

Just on bug to fix and a couple of comments/very small improvements.

anarnoli · 2023-05-15T18:39:19Z

@wayfarer3130 Observations fixed, request to review.

feat: query string dynamic app config

29f079d

anarnoli mentioned this pull request May 14, 2023

feat: query string dynamic app config #2925

Closed

5 tasks

wayfarer3130 reviewed May 15, 2023

View reviewed changes

wayfarer3130 requested changes May 15, 2023

View reviewed changes

fix: bug fix and couple of very small improvements

3281fdc

wayfarer3130 approved these changes May 15, 2023

View reviewed changes

wayfarer3130 merged commit af8bb27 into OHIF:v3-stable May 15, 2023
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: query string dynamic app config #3391

feat: query string dynamic app config #3391

anarnoli commented May 14, 2023

netlify bot commented May 14, 2023 •

edited

netlify bot commented May 14, 2023 •

edited

codecov bot commented May 14, 2023 •

edited

anarnoli commented May 14, 2023

wayfarer3130 May 15, 2023

wayfarer3130 May 15, 2023

anarnoli May 15, 2023

wayfarer3130 May 15, 2023

wayfarer3130 May 15, 2023

wayfarer3130 May 15, 2023

anarnoli May 15, 2023

wayfarer3130 left a comment

anarnoli commented May 15, 2023

feat: query string dynamic app config #3391

feat: query string dynamic app config #3391

Conversation

anarnoli commented May 14, 2023

Context

Checklist

PR

Code

Public Documentation Updates

netlify bot commented May 14, 2023 • edited

✅ Deploy Preview for ohif-platform-docs ready!

netlify bot commented May 14, 2023 • edited

✅ Deploy Preview for ohif-platform-viewer ready!

codecov bot commented May 14, 2023 • edited

Codecov Report

anarnoli commented May 14, 2023

wayfarer3130 May 15, 2023

Choose a reason for hiding this comment

wayfarer3130 May 15, 2023

Choose a reason for hiding this comment

anarnoli May 15, 2023

Choose a reason for hiding this comment

wayfarer3130 May 15, 2023

Choose a reason for hiding this comment

wayfarer3130 May 15, 2023

Choose a reason for hiding this comment

wayfarer3130 May 15, 2023

Choose a reason for hiding this comment

anarnoli May 15, 2023

Choose a reason for hiding this comment

wayfarer3130 left a comment

Choose a reason for hiding this comment

anarnoli commented May 15, 2023

netlify bot commented May 14, 2023 •

edited

netlify bot commented May 14, 2023 •

edited

codecov bot commented May 14, 2023 •

edited