Skip to content

Conversation

@ahoseinian
Copy link
Contributor

For example if the param was i_key the current regex would not find it as a variable. it was only working for the params with more than one letter before either . - or _. the fix should allow to find params with only one letter at begninng.

@nicky1038
Copy link
Contributor

@ahoseinian Hi!

I've also found that the existing regex does not detect all path params within route path string. One-letter params (like a) are also not being detected. Your suggested regex doesn't detect them too, so I would suggest another variant:

/({[a-zA-Z]([-_.]?[a-zA-Z0-9])*})|(:[a-zA-Z]([-_.]?[a-zA-Z0-9])*:?)/g

There are other improvements here:

  • it is shorter and better explains what it does
    • redundant ([0-9]+)? in the end, {1}, brackets were removed
    • -?_?\.? replaced to [-_.]?
  • the first letter should be [a-zA-Z], not [A-z].

ahoseinian added a commit to ahoseinian/swagger-typescript-api that referenced this pull request Apr 28, 2024
@ahoseinian
Copy link
Contributor Author

hi @nicky1038,

Thanks for the suggestion. I wanted to change this minimally so not to break anything, but as the tests are all passing, I think this should be fine.

just one small problem I noticed with your version of the regex. it would not find parameters with two underlines. which I guess it is unlikely but as the previous version would I wouldn't want to break that. what I mean is for example a param like {i_key} would be found in the url with no problem but {i__key} not anymore.

so I will change this to:
/({[a-zA-Z]([a-zA-Z0-9-_.])*})|(:[a-zA-Z]([-_.]?[a-zA-Z0-9-_.])*:?)/g

let me know if you have any suggestions.

@nicky1038
Copy link
Contributor

nicky1038 commented Apr 30, 2024

@ahoseinian Trying to change as little code as possible in order to prevent it from breaking is a very rational idea. However, the case of swagger-typescript-api is not typical, as in the last major release the author themselves changed this regex, so that new bugs appeared. IMHO this regex needed refactoring even in v12, and now it is even better time to perform it.

My regex doesn't find {i__key} because both original regexes in v12 (/({(([a-zA-Z]-?_?\.?){1,})([0-9]{1,})?})|(:(([a-zA-Z]-?_?\.?){1,})([0-9]{1,})?:?)/g) and v13 (/({(([A-z]){1}([a-zA-Z0-9]-?_?\.?)+)([0-9]+)?})|(:(([A-z]){1}([a-zA-Z0-9]-?_?\.?)+)([0-9]+)?:?)/g) don't find such parameter too. Perhaps it is wrong behavior, and we should handle this situation too — so it's one more argument in favor of complete refactoring of the regex :)

My thoughts about your latest suggestion:

  • should we allow trailing delimiter signs -, _ or .? (I don't think it's needed)
  • the second part of the regex was not fully updated according to the first one

So depending on what we decide, it would be either

/({[a-zA-Z]([-_.]*[a-zA-Z0-9])*})|(:[a-zA-Z]([-_.]*[a-zA-Z0-9])*:?)/g

or

/({[a-zA-Z]([a-zA-Z0-9-_.])*})|(:[a-zA-Z]([a-zA-Z0-9-_.])*:?)/g

I'd prefer the first option.

ahoseinian added a commit to ahoseinian/swagger-typescript-api that referenced this pull request May 10, 2024
For example if the param was i_key the current regex would not find it as a variable. it was only working for the params with more than one letter before either `.` `-` or `_`. the fix should allow to find params with only one letter at begninng.
@ahoseinian ahoseinian force-pushed the bugfix/fix-route-path-params-parsing branch from 9be7ea8 to 7a8f234 Compare May 10, 2024 15:13
@ahoseinian
Copy link
Contributor Author

@nicky1038 totally agree:

updated the regex to your suggested one:

/({[a-zA-Z]([-_.]*[a-zA-Z0-9])*})|(:[a-zA-Z]([-_.]*[a-zA-Z0-9])*:?)/g

@ahoseinian
Copy link
Contributor Author

@smorimoto saw that you have put some commits recently. maybe you can take a look at this also :)

@smorimoto
Copy link
Collaborator

Could you rebase to latest main branch?

@smorimoto smorimoto added the bug Something isn't working label Jun 27, 2024
@smorimoto smorimoto requested a review from js2me June 27, 2024 22:58
@smorimoto smorimoto requested a review from Copilot November 20, 2024 00:37
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 4 changed files in this pull request and generated no suggestions.

Files not reviewed (1)
  • tests/spec/extractRequestParams/schema.json: Language not supported
Comments skipped due to low confidence (3)

tests/spec/extractRequestParams/schema.ts:475

  • [nitpick] The variable name 'iKey' is ambiguous. It should be renamed to 'authentiqKey'.
iKey = {

tests/spec/extractRequestParams/schema.ts:483

  • [nitpick] The variable name 'iPk' is ambiguous. It should be renamed to 'authentiqKeyId'.
iKeyDetail: (iPk: string, params: RequestParams = {}) =>

src/schema-routes/schema-routes.js:113

  • Ensure that the new regex pattern is covered by tests, specifically for route parameters that begin with a single letter.
/({[a-zA-Z]([-_.]*[a-zA-Z0-9])*})|(:[a-zA-Z]([-_.]*[a-zA-Z0-9])*:?)/g,

@nicky1038
Copy link
Contributor

@smorimoto I can see there's been no activity here for a long time. So I've rebased these changes myself (without any conflicts). The new PR is #1064. I hope this helps to deliver them into production sooner.

@ahoseinian I hope you don't mind this :) I've mentioned you in the new PR.

@ahoseinian
Copy link
Contributor Author

@nicky1038 Thank you. somehow I forgot to continue with this.

@ahoseinian ahoseinian closed this Feb 28, 2025
smorimoto pushed a commit to nicky1038/swagger-typescript-api that referenced this pull request Mar 22, 2025
smorimoto pushed a commit to nicky1038/swagger-typescript-api that referenced this pull request Mar 22, 2025
smorimoto added a commit that referenced this pull request Mar 22, 2025
…parsing

Fix route param parsing when the param begins with just one letter (rebased original PR #686 by @ahoseinian)
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.

3 participants