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

Apply encodeURIComponent on path parameter value #1696

Merged
merged 4 commits into from
Jun 24, 2024
Merged

Conversation

swachter
Copy link
Contributor

Changes

Fixes #1596

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

  • Unit tests updated
  • docs/ updated (if necessary)
  • pnpm run update:examples run (only applicable for openapi-typescript)

Copy link

changeset-bot bot commented Jun 17, 2024

🦋 Changeset detected

Latest commit: 2f853c0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
openapi-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@drwpow
Copy link
Contributor

drwpow commented Jun 17, 2024

Good addition, but could you add a test (preferably one that fails before this change)? Thanks!

@StefanWachter1507
Copy link
Contributor

I am sorry. I am unable to provide a test.

I have difficulties to work on the project. I installed pnpm and run pnpm i. Yet, the test fails.

> openapi-typescript-monorepo@0.0.0 test /home/stwachte/projects/eigene/openapi-typescript
> pnpm run -r --parallel --aggregate-output test

Scope: 8 of 9 workspace projects
packages/openapi-fetch test$ pnpm run "/^test:/"
packages/openapi-typescript test$ pnpm run "/^test:/"
packages/openapi-typescript-helpers test$ tsc --noEmit
packages/openapi-typescript test:  ERR_PNPM_NO_SCRIPT  Missing script: /^test:/
packages/openapi-typescript test: Failed
/home/stwachte/projects/eigene/openapi-typescript/packages/openapi-typescript:
 ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL  openapi-typescript@7.0.0-rc.0 test: `pnpm run "/^test:/"`
Exit status 1
 ELIFECYCLE  Test failed. See above for more details.

I tried it both, running pnpm test in the root folder and in the openapi-fetch folder. In addition, IntelliJ-Ultimate does not understand the project structure properly.

@drwpow
Copy link
Contributor

drwpow commented Jun 21, 2024

The package’s CONTRIBUTING guide has some basic instructions on test setup. You can also inspect .github/workflows/ci.yml to see the exact commands CI runs (which happens on a clean environment so it contains all the setup)

Also make sure you have pnpm 9 (current) and Node 22 (current) installed.

@kerwanp kerwanp requested a review from a team as a code owner June 23, 2024 10:16
@StefanWachter1507
Copy link
Contributor

I finally managed to run tests and found that the params / paths / allows UTF-8 characters test failed after my changes. IMHO that test should have failed because the question mark character in a path parameter should not be interpreted as the query string separator. I fixed that test to expect the path parameter to be url-encoded.

@drwpow
Copy link
Contributor

drwpow commented Jun 24, 2024

Ah yes you’re right! Good catch—I agree that’s an improvement.

This PR still needs a test unique to your change, though! Preferably one that fails before your change.

@StefanWachter1507
Copy link
Contributor

I added two more tests and changed the existing "allows UTF-8 characters" test slightly.

Copy link
Contributor

@drwpow drwpow left a comment

Choose a reason for hiding this comment

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

Great work, thank you!

@drwpow drwpow merged commit aea31a8 into openapi-ts:main Jun 24, 2024
7 checks passed
@github-actions github-actions bot mentioned this pull request Jun 24, 2024
@StefanWachter1507
Copy link
Contributor

I am glad that I could help.

When will this be released? I will have to remove a workaround in our code base and would prefer to do it soon.

@drwpow
Copy link
Contributor

drwpow commented Jun 24, 2024

When will this be released?

It’s being published right now 🙂. Thanks to changesets, the release process is automated

kerwanp pushed a commit to kerwanp/openapi-typescript that referenced this pull request Jul 29, 2024
* Apply encodeURIComponent on path parameter value

* add changeset

* Fix "params / path / allows UTF-8 characters" test

* Add more tests for path parameter encoding

---------

Co-authored-by: Stefan Wachter <stefan.wachter@otto.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

defaultPathSerializer doesn't url-encode its primitive string params
3 participants