Skip to content

fix(frontend): double prefixed API url - #35641

Closed
jakubhruby wants to merge 1 commit into
apache:masterfrom
jakubhruby:fix/postFormUrl
Closed

jakubhruby wants to merge 1 commit into
apache:masterfrom
jakubhruby:fix/postFormUrl

Conversation

@jakubhruby

Copy link
Copy Markdown
Contributor

SUMMARY

When app root prefix is applied (e.g. SUPERSET_APP_ROOT="/app-prefix"), it's applied twice - e.g. when exporting graph to CSV

// in this handler url is build with appRoot prefix
exportChart = async (...) => {
    ...
    url = ensureAppRoot('/api/v1/chart/data'); 
    // now the url is /app-prefix/api/v1/chart/data

    // postForm is called with prefixed url
    SupersetClient.postForm(url, ...)
    ...
}

// this method is called with already prefixed url
async postForm(endpoint, ...) {
    ...
    hiddenForm.action = this.getUrl({ endpoint });
    // now the url is /app-prefix/app-prefix/api/v1/chart/data
    ...
}

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Snímek obrazovky z 2025-10-14 14-53-35

TESTING INSTRUCTIONS

Test downloading CSV file.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@korbit-ai korbit-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.
Category Issue Status
Functionality Unescaped regex pattern in applicationRoot replacement ▹ view
Performance Inefficient regex creation in hot path ▹ view
Files scanned ​
File Path Reviewed
superset-frontend/src/explore/exploreUtils/index.js ✅
superset-frontend/src/components/Chart/chartAction.js ✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

endpointType,
allowDomainSharding: false,
});
}).replace(new RegExp(`^${applicationRoot()}`), ''); // appRoot will be added in SuperstClient.postForm method

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unescaped regex pattern in applicationRoot replacement category Functionality

Tell me more
What is the issue?

The regex replacement uses an unescaped applicationRoot() value which could contain special regex characters, potentially causing incorrect string replacement or regex errors.

Why this matters

If applicationRoot() returns a string containing regex metacharacters like '.', '+', '*', '?', '^', '$', '(', ')', '[', ']', '{', '}', '|', '', the regex will not match the intended literal string and may cause unexpected behavior or runtime errors.

Suggested change ∙ Feature Preview

Escape the applicationRoot() value before using it in the regex pattern:

}).replace(new RegExp(`^${applicationRoot().replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}`), ''); // appRoot will be added in SuperstClient.postForm method

Alternatively, use a simpler string-based approach:

const appRoot = applicationRoot();
if (url.startsWith(appRoot)) {
  url = url.substring(appRoot.length);
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

endpointType,
allowDomainSharding: false,
});
}).replace(new RegExp(`^${applicationRoot()}`), ''); // appRoot will be added in SuperstClient.postForm method

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inefficient regex creation in hot path category Performance

Tell me more
What is the issue?

Regular expression is created on every function call instead of being cached or using a more efficient string operation.

Why this matters

Creating a new RegExp object on each function execution adds unnecessary overhead, especially if exportChart is called frequently. This impacts performance through repeated object instantiation and regex compilation.

Suggested change ∙ Feature Preview

Cache the regex pattern outside the function or use string methods like startsWith() and slice() for better performance:

const APP_ROOT_REGEX = new RegExp(`^${applicationRoot()}`);
// Then use: url.replace(APP_ROOT_REGEX, '')
// Or better: url.startsWith(appRoot) ? url.slice(appRoot.length) : url
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

@dosubot dosubot Bot added change:frontend Requires changing the frontend viz:charts:export Related to exporting charts labels Oct 14, 2025
@sadpandajoe sadpandajoe changed the title fix(frontend) double prefixed API url fix(frontend): double prefixed API url Oct 14, 2025
@rusackas

Copy link
Copy Markdown
Member

@jakubhruby — friendly ping on this one. It's been sitting for a few months and has picked up some blockers:

  • Merge conflicts against master — needs a rebase.
  • CI is currently failing.
  • A couple of korbit-ai suggestions on superset-frontend/src/explore/exploreUtils/index.js:262 are still open (regex escaping + avoiding RegExp creation in a hot path) — worth a look while you're in there, though they're optional.

If you're still interested in landing this fix, could you rebase and push? Otherwise we can close it out and revisit later. No rush — just trying to keep the queue tidy.

@jakubhruby

Copy link
Copy Markdown
Contributor Author

Duplicated and replaced by #38513

@jakubhruby

Copy link
Copy Markdown
Contributor Author

closing

@jakubhruby jakubhruby closed this Apr 27, 2026
@jakubhruby
jakubhruby deleted the fix/postFormUrl branch April 27, 2026 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend size/XS viz:charts:export Related to exporting charts

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants