Skip to content

gene's encode_uri added/adapted to python - #408

Open
ATorrise wants to merge 8 commits into
mainfrom
gene-encoding-of-uri
Open

gene's encode_uri added/adapted to python#408
ATorrise wants to merge 8 commits into
mainfrom
gene-encoding-of-uri

Conversation

@ATorrise

@ATorrise ATorrise commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

What It Does

Ports the URI-encoding fixes from Gene's Zowe CLI PR zowe/zowe-cli#2758 to the Python SDK, adding EncodeUri logic

  • Mirrors the 2-pathed encoding differences/strictness between uss (_encode_uri_path_for_uss) and ds(with ds being less restrictive)(_encode_uri_path_for_zos)
  • Includes bug fix found when porting:
    • Console.__init__ put the literal string "defcn" (the default console name) right into the base URL, then str.replace replaced every instance of defcn in the string, so if for some reason there was a valid defcn in the URL, that would get replaced and break the URL

How to Test

Review Checklist
I certify that I have:

  • updated the changelog
  • manually tested my changes
  • added/updated automated unit/integration tests
  • created/ran system tests (provide build number if applicable)
  • followed the contribution guidelines

Additional Comments

Signed-off-by: Amber Torrise <ambertorrise@gmail.com>
@github-project-automation github-project-automation Bot moved this to New Issues in Zowe CLI Squad Jul 30, 2026
@zowe-robot zowe-robot moved this from New Issues to In Progress in Zowe CLI Squad Jul 30, 2026
@ATorrise ATorrise changed the title genes fixes added and adapted to python gene's encode_uri added/adapted to python Jul 31, 2026
@ATorrise
ATorrise marked this pull request as ready for review August 5, 2026 15:09
@zowe-robot zowe-robot moved this from In Progress to Review/QA in Zowe CLI Squad Aug 5, 2026
@codecov

codecov Bot commented Aug 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.82%. Comparing base (91f8021) to head (ad48c77).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #408      +/-   ##
==========================================
+ Coverage   83.06%   84.82%   +1.75%     
==========================================
  Files          49       49              
  Lines        2982     3011      +29     
==========================================
+ Hits         2477     2554      +77     
+ Misses        505      457      -48     
Flag Coverage Δ
unittests 84.82% <100.00%> (+1.75%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pujal0909 and others added 3 commits August 6, 2026 16:06
Signed-off-by: ATorrise <ambertorrise@gmail.com>
Signed-off-by: ATorrise <ambertorrise@gmail.com>
@ATorrise
ATorrise requested review from CBforZ, awharn, jace-roell, pujal0909, t1m0thyj, traeok and zFernand0 and removed request for zFernand0 August 7, 2026 15:30
Signed-off-by: ATorrise <ambertorrise@gmail.com>
Comment on lines +215 to +216
if next_char in _USS_CHARS_TO_ENCODE:
encoded_path.append(_USS_CHARS_TO_ENCODE[next_char])

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since % is included in the map of _USS_CHARS_TO_ENCODE, if a URI contains characters that are already encoded such as %20 for space, it will get converted to %2520 which is not valid.

Not sure if it's in scope to fix in this PR, given that the implementation in the Node SDK seems to have the same problem, and the input passed to this method should typically not be encoded yet. Thoughts @zFernand0 @traeok?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Would this mean that merging would cause a regression in behavior given the scope of expected URIs?
I think we could address this separately since the Node.js SDK already suffers from the same problem - but since we've now identified it can be an issue, maybe we can proactively add a note for developers to clarify that pre-encoded URIs are not accepted (until this is resolved).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Filed this as a separate issue (we can label as low priority) #413

@zFernand0 zFernand0 self-assigned this Aug 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Review/QA

Development

Successfully merging this pull request may close these issues.

7 participants