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

Fix loading session shares that have a plus sign in the sessionId #3524

Merged
merged 2 commits into from
Feb 16, 2023

Conversation

cmdcolin
Copy link
Collaborator

@cmdcolin cmdcolin commented Feb 16, 2023

Fixes #3520

Works

https://share.jbrowse.org/api/v1/load?sessionId=Xc-hrJg1O%2B

Fails

https://share.jbrowse.org/api/v1/load?sessionId=Xc-hrJg1O+

Seems like it should be the same, could fix on the server side perhaps, but this is a client side fix

@github-actions github-actions bot added the needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) label Feb 16, 2023
@cmdcolin
Copy link
Collaborator Author

Loading the example share link from #3520 works on this branch now

@cmdcolin cmdcolin added bug Something isn't working and removed needs label triage Needs a label to show in changelog (breaking, enhancement, bug, documentation, or internal) labels Feb 16, 2023
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Merging #3524 (13eb956) into main (819526f) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 13eb956 differs from pull request most recent head 11776cd. Consider uploading reports for the commit 11776cd to get more accurate results

@@           Coverage Diff           @@
##             main    #3524   +/-   ##
=======================================
  Coverage   62.57%   62.57%           
=======================================
  Files         862      862           
  Lines       30164    30164           
  Branches     7234     7234           
=======================================
  Hits        18875    18875           
  Misses      11101    11101           
  Partials      188      188           
Impacted Files Coverage Δ
products/jbrowse-web/src/sessionSharing.ts 23.07% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@cmdcolin
Copy link
Collaborator Author

This PR now updates the lambda to replace space char with a plus in the sessionId, to work around the plus sign being interpreted as a space. It also encodesURIComponent too since this is probably proper behavior anyways

@cmdcolin cmdcolin merged commit b645ef1 into main Feb 16, 2023
@cmdcolin cmdcolin deleted the fix_session_sharing_dynamo branch February 16, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Session sharing lambda can create characters that aren't properly shareable in rare cases
1 participant