swagger-typescript-api icon indicating copy to clipboard operation
swagger-typescript-api copied to clipboard

Fix route param parsing when the param begins with just one letter

Open ahoseinian opened this issue 1 year ago • 6 comments

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 avatar Feb 22 '24 14:02 ahoseinian

@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].

nicky1038 avatar Apr 19 '24 19:04 nicky1038

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.

ahoseinian avatar Apr 28 '24 14:04 ahoseinian

@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.

nicky1038 avatar Apr 30 '24 10:04 nicky1038

@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 avatar May 10 '24 15:05 ahoseinian

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

ahoseinian avatar May 10 '24 15:05 ahoseinian