feat: Impossible to use UUID as param for object construction #706 #708

ivanproskuryakov · 2021-04-13T08:52:58Z

Description

The usage of UUID instead of id in routes like http://0.0.0.0:3000/api/products/04879b32-c329-48d7-a652-818794b684f1
Is trying to build JSON object while returning a raw parameter(UUID) provided.

Ref: #703

Checklist

the pull request title describes what this PR does (not a vague title like Update index.md)
the pull request targets the default branch of the repository (develop)
the code follows the established code style of the repository
- npm run prettier:check passes
- npm run lint:check passes
[s] tests are added for the changes I made (if any source code was modified)
documentation added or updated
I have run the project locally and verified that there are no errors

Fixes

The conditions the inside normalizeParamValue now excluding "param" type, keeping UUID strings untouched

qdanik · 2021-04-14T00:44:44Z

src/ActionParameterHandler.ts

      typeof value === 'object' &&
-      ['queries', 'headers', 'params', 'cookies'].some(paramType => paramType === param.type)
-    ) {
+      ['queries', 'headers', 'params', 'cookies'].some(paramType => paramType === param.type);


includes() would be better in this case

resolved https://github.com/typestack/routing-controllers/pull/708/files#diff-e388c22d7bdf11f58c29a3cf56e5fbf79de2efad1cbc8f0c54dc854cf146ca67R100

qdanik · 2021-04-14T00:45:21Z

src/ActionParameterHandler.ts

-      ['queries', 'headers', 'params', 'cookies'].some(paramType => paramType === param.type)
-    ) {
+      ['queries', 'headers', 'params', 'cookies'].some(paramType => paramType === param.type);
+    const isTargetPrimitive = ['number', 'string', 'boolean'].indexOf(param.targetName) > -1;


Also use includes() here

resolved https://github.com/typestack/routing-controllers/pull/708/files#diff-e388c22d7bdf11f58c29a3cf56e5fbf79de2efad1cbc8f0c54dc854cf146ca67R101

qdanik · 2021-04-14T00:46:00Z

test/ActionParameterHandler.spec.ts

+const expect = require('chakram').expect;
+
+describe('ActionParameterHandler', () => {
+  it('handle', async () => {


You should add negative and positive cases

Broader test coverage was added including two negative tests:

https://github.com/typestack/routing-controllers/pull/708/files#diff-5c248b2b7632d75cc5c2d989e5b9387fe961cb04f00ef500c93b55ccc93e1a15R128

https://github.com/typestack/routing-controllers/pull/708/files#diff-5c248b2b7632d75cc5c2d989e5b9387fe961cb04f00ef500c93b55ccc93e1a15R148

PASS test/ActionParameterHandler.spec.ts
ActionParameterHandler
✓ handle - should process string parameters (154 ms)
✓ handle - should process string parameters, returns empty if a given string is empty
✓ handle - should process number parameters (1 ms)
✓ handle - undefined on empty object provided
✓ handle - throws error if the parameter is required
✓ handle - throws error if the parameter is required, type file provided (10 ms)

…provements

…ive cases

qdanik · 2021-04-18T22:27:06Z

test/ActionParameterHandler.spec.ts

+  });
+
+  it('handle - throws error if the parameter is required', async () => {
+    const driver = new ExpressDriver();


You should wrap these negative cases with describe('negative', () => {}) and add beforeEach with

const driver = new ExpressDriver(); const actionParameterHandler = new ActionParameterHandler(driver); const param = buildParamMetadata('uuid', 'file', true);

qdanik · 2021-04-18T22:31:41Z

test/ActionParameterHandler.spec.ts

+  });
+
+  it('handle - should process string parameters, returns empty if a given string is empty', async () => {
+    const driver = new ExpressDriver();


Look at my comment above and do the same with describe('positive', () => {})

But, these 3 positive cases have only two common variables:

const driver = new ExpressDriver(); const actionParameterHandler = new ActionParameterHandler(driver);

@qdanik thanks for the review and valuable feedback!
Could you take a look once again, please?

@ivanproskuryakov Looks good for me. Please add @NoNameProvided into Reviewers

@qdanik can't do so myself, this functionality is not available to me.

qdanik

Looks good for me

ivanproskuryakov · 2021-05-31T13:30:02Z

@qdanik @nolazybits @fabiob could the PR be merged, please?

ivanproskuryakov · 2021-06-11T13:59:51Z

@nolazybits @jvelo @fabiob ping 📦

fabiob · 2021-06-11T14:26:34Z

LGTM, but I don't have write access.

ivanproskuryakov · 2021-07-04T10:00:45Z

@MichalLytek @jotamorais @NoNameProvided @rustamwin hi, can we merge this PR?

src/ActionParameterHandler.ts

Co-authored-by: Rustam Mamadaminov <rmamdaminov@gmail.com>

rustamwin · 2021-07-20T07:59:35Z

src/ActionParameterHandler.ts

-      typeof value === 'object' &&
-      ['queries', 'headers', 'params', 'cookies'].some(paramType => paramType === param.type)
-    ) {
+    if (isNormalisationNeeded) {


Suggested change

if (isNormalisationNeeded) {

if (isNormalizationNeeded) {

ivanproskuryakov · 2021-07-20T12:39:07Z

@rustamwin closed because to merging conflicts, the new PR - #753

github-actions · 2021-08-20T00:09:11Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

ivanproskuryakov added 2 commits April 13, 2021 11:52

fix: uuid support added with test

b03d7b4

fix: cs & linter

5701639

ivanproskuryakov mentioned this pull request Apr 13, 2021

feat: Impossible to use UUID as param for object construction #706

Closed

5 tasks

qdanik reviewed Apr 14, 2021

View reviewed changes

ivanproskuryakov added 3 commits April 15, 2021 12:44

fix: adding more test cases according to the feedback provided, cs im…

1a3618b

…provements

fix: adding more test cases according to the feedback provided, negat…

9861f71

…ive cases

fix: fixing cs with running prettier:fix

239fb0d

qdanik reviewed Apr 18, 2021

View reviewed changes

fix: code review ref: typestack#708 (review)

c4bb9c8

qdanik previously approved these changes Apr 20, 2021

View reviewed changes

fabiob previously approved these changes May 31, 2021

View reviewed changes

noeoliveira previously approved these changes Jun 21, 2021

View reviewed changes

rustamwin previously approved these changes Jul 7, 2021

View reviewed changes

src/ActionParameterHandler.ts Outdated Show resolved Hide resolved

Update src/ActionParameterHandler.ts

6af1287

Co-authored-by: Rustam Mamadaminov <rmamdaminov@gmail.com>

ivanproskuryakov dismissed stale reviews from rustamwin, noeoliveira, fabiob, and qdanik via 6af1287 July 20, 2021 07:53

rustamwin suggested changes Jul 20, 2021

View reviewed changes

ivanproskuryakov force-pushed the uuid-parameters-703 branch 2 times, most recently from 33a38c5 to 2a3fa2b Compare July 20, 2021 12:08

fix: Update src/ActionParameterHandler.ts

039c89f

ivanproskuryakov force-pushed the uuid-parameters-703 branch from 2a3fa2b to 039c89f Compare July 20, 2021 12:27

ivanproskuryakov closed this Jul 20, 2021

ivanproskuryakov deleted the uuid-parameters-703 branch July 20, 2021 12:32

ivanproskuryakov mentioned this pull request Jul 20, 2021

feat: Impossible to use UUID as param for object construction #706 #753

Merged

5 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Impossible to use UUID as param for object construction #706 #708

feat: Impossible to use UUID as param for object construction #706 #708

ivanproskuryakov commented Apr 13, 2021

qdanik Apr 14, 2021

ivanproskuryakov Apr 15, 2021

qdanik Apr 14, 2021

ivanproskuryakov Apr 15, 2021

qdanik Apr 14, 2021

ivanproskuryakov Apr 15, 2021 •

edited

qdanik Apr 18, 2021

qdanik Apr 18, 2021 •

edited

qdanik Apr 18, 2021

ivanproskuryakov Apr 19, 2021

qdanik Apr 20, 2021

ivanproskuryakov Apr 20, 2021

qdanik left a comment

ivanproskuryakov commented May 31, 2021

ivanproskuryakov commented Jun 11, 2021

fabiob commented Jun 11, 2021

ivanproskuryakov commented Jul 4, 2021

rustamwin Jul 20, 2021

ivanproskuryakov commented Jul 20, 2021

github-actions bot commented Aug 20, 2021

feat: Impossible to use UUID as param for object construction #706 #708

feat: Impossible to use UUID as param for object construction #706 #708

Conversation

ivanproskuryakov commented Apr 13, 2021

Description

Checklist

Fixes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanproskuryakov Apr 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdanik Apr 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qdanik left a comment

Choose a reason for hiding this comment

ivanproskuryakov commented May 31, 2021

ivanproskuryakov commented Jun 11, 2021

fabiob commented Jun 11, 2021

ivanproskuryakov commented Jul 4, 2021

Choose a reason for hiding this comment

ivanproskuryakov commented Jul 20, 2021

github-actions bot commented Aug 20, 2021

ivanproskuryakov Apr 15, 2021 •

edited

qdanik Apr 18, 2021 •

edited