Restrict the allowed types of path parameters

[balazsbotond]

Only the following should be allowed as path parameter values:

• string
• number
• boolean - I'm not sure about this one though but I don't want to be overly strict

The following primitive types, in my opinion, should not be allowed:

• symbol
• bigint
• undefined
• null

Complex types (arrays and object) should not be allowed at all.

The error thrown should be a TypeError and its message should contain the parameter name and a list of allowed types.

[harshilparmar]

Can I try this ?

[balazsbotond]

@harshilparmar sure, it's yours! :)

[harshilparmar]

`function path(template: string, params: ParamMap) { const remainingParams = { ...params }; const renderedPath = template.replace(/:\w+/g, p => { const key = p.slice(1); if (!params.hasOwnProperty(key)) { return p; } delete remainingParams[key]; const allowedTypes = ["boolean","string","number"]; if(allowedTypes.includes(typeof params[key])){ return encodeURIComponent(params[key]); }else{ throw new TypeError("${typeof params[key]} is not Valid,Params must be number,string or boolean") } });

return { renderedPath, remainingParams }; }` @balazsbotond is this correct way? Please correct me If I am wrong 🙇‍♂️

[balazsbotond]

@harshilparmar thank you, I believe this is correct 👍 I like the names you used and the overall logic of your code.

Just a couple of minor, mostly stylistic suggestions:

• You can negate the condition in the if to reduce the number of nested blocks (so it would become if (!allowedTypes.includes...) { throw ...; } and then just return, no need for an else block
• I would move the allowedTypes above the template.replace call so that it isn't redefined every time a parameter is matched
• Please use single quotes for the strings to be consistent with the rest of the project
• I would rather use an error text like this: Path parameter ${key} cannot be of type ${typeof params[key]}. Allowed types are: ${allowedTypes.join(', ')}.. This includes a bit more detail about the error which makes debugging easier.

Could you please create a pull request with your chages? This would make it easier to review the code.

Also, could you please include tests in your PR - I think they need to be added for the subst and urlcat functions.

Thanks again - I'm very glad you're interested in the project and took the time to contribute code!

[harshilparmar]

@balazsbotond Thanks for suggestions brother!!! I will make pr with this changes.I would love to contribute and learn from you. Thanks for 1st and last points.For 2ed and 3rd point I changed code accordingly only to show you,HAHAHAH But it was right.Thanks brother 💯

[harshilparmar]

@balazsbotond Would you like to show specific type of objects??

https://stackoverflow.com/questions/7893776/the-most-accurate-way-to-check-js-objects-type https://javascriptweblog.wordpress.com/2011/08/08/fixing-the-javascript-typeof-operator/

[balazsbotond]

@harshilparmar I agree that in general, just the string "object" is not very informative - and in most cases it would be a good idea to also return the name of the type.

But almost all objects passed to urlcat will be object literals - for those these solutions would still return "object" or "Object" - so in this specific case, I don't think it's worth adding more code to return the exact type.

[harshilparmar]

@harshilparmar I agree that in general, just the string "object" is not very informative - and in most cases it would be a good idea to also return the name of the type.

But almost all objects passed to urlcat will be object literals - for those these solutions would still return "object" or "Object" - so in this specific case, I don't think it's worth adding more code to return the exact type.

@balazsbotond ohh, for that we don't need to add that💯

[harshilparmar]

@balazsbotond Can you help me to figure out how to write test case for TypeError? I have made something like this it('Don\'t allow null as parameter', () => { const actual = subst(':p', { p: null }); expect(actual).to.throw(new TypeError('Path parameter p cannot be of type object.Allowed types are: boolean, string, number.')); });

But it's not passing test !!

[balazsbotond]

@harshilparmar Null and undefined values are removed in urlcatImpl before calling path (by design - though I'm planning to change this in the future) so that's why your code won't throw an exception in that case. I could have explained this better in my original comment on this issue, sorry about that.

So the solution is simply not writing a test case for null and undefined.

[balazsbotond]

Categorized this as a bug instead of an enhancement because it fixes unexpected behavior.

Closing.

[balazsbotond]

@all-contributors please add @harshilparmar for code

[allcontributors[bot]]

@balazsbotond

I've put up a pull request to add @harshilparmar! :tada: