Fix missing @QueryParams decorator options #270

MichalLytek · 2017-08-31T17:31:30Z

This PR add param options for @QueryParams() decorator to let enable/disable validation/transforming and other stuffs.

Also it introduce normalizing of query params object properties like it's already done for simple @QueryParam decorator.

Fixes #160.

MichalLytek · 2017-08-31T17:48:10Z

src/ActionParameterHandler.ts

+        if (param.type === "queries" && typeof value === "object") {
+            Object.keys(value).map(key => {
+                const type = Reflect.getMetadata("design:type", param.targetType.prototype, key);
+                const typeString = typeof type();


TBH, I don't like this part, I did this as a quick shortcut.
It may introduce some weird behavior when the reflected type is not constructor-like (is it possible in TypeScript?) especially when it's undefined, so I think I should check if the type is undefined for case like no property type provided in class definition.

MichalLytek · 2017-08-31T18:11:41Z

src/ActionParameterHandler.ts

+
+                return Boolean(value);
+
+            default:


I think we should also cover the date case and also the array feature which was requested in #123 - it is possible with class-transform @Type decorator to work with arrays.

NoNameProvided · 2017-08-31T21:24:08Z

src/ActionParameterHandler.ts

+        switch (type) {
+            case "number":
+                if (value === "")
+                    return undefined;


I think if a parameter is passed in the query with no value it should be casted to null instead of undefined.

I will add a normal comment to detail this.

I've just extracted pleerock's code to separate method, I haven't changed nothing in normalizing logic.
https://github.com/pleerock/routing-controllers/blob/master/src/ActionParameterHandler.ts#L112

I agree that we should convert "" to true for boolean, but for number we should use undefined if param value not exist or NaN if it's not a number string. However I would like to don't mix different things in one PR, as we first need to discuss the edge cases we should merge this @QueryParams PR and start a new one with better normalizing.

NoNameProvided · 2017-08-31T21:24:19Z

I think if a parameter is passed in the query with no value it should be casted to null instead of undefined.

?paramA=3&paramB should not be equal to ?paramA=3. For this reason I would return null instead of undefined. This should also apply to all the other types (boolean, string).

Also this current approach doesn't allow partial description of the queryparams because of the incorrect behaviour of IsOptional decorator. (See typestack/class-validator/issues/101 for details.) Luckily there is a fix for that but it has not been released yet. And an another fix would be needed also to allow the acceptance of empty strings as pointed out in the comments.

So now if a query class is defined every key must exists, otherwise the validation will fail right?

Example:

class GetUsersQuery {
 
     @IsOptional()
     @IsPositive()
     limit: number;
 
     @IsOptional()
     @IsAlpha()
     city: string;
 
     @IsOptional()
     @IsEnum(Roles)
     role: string;
 
     @IsOptional()
     @IsBoolean()
     isActive: boolean;
 
 }
 
// this will work for query `?limit=5&city=Amsterdam&role=moderator&isActive=true`
// but wont work for `?limit=5&role=moderator`

So to make this work properly we have to pass the following test as well:

describe("@QueryParam should give a proper values from request query parameters", () => {
        assertRequest([3001, 3002], "get", "photos?sortBy=name&limit=10&showAll", response => {
            expect(queryParamSortBy).to.be.equal("name");
            expect(queryParamCount).to.be.equal(undefined);
            expect(queryParamLimit).to.be.equal(10);
            expect(queryParamShowAll).to.be.equal(null);
            expect(response).to.be.status(200);
            expect(response).to.have.header("content-type", "text/html; charset=utf-8");
        });
    });

NoNameProvided · 2017-08-31T21:30:12Z

Another idea could be to support default params?

Example:

class GetUsersQuery {
 
     @IsOptional()
     @IsPositive()
     limit: number = 30
 
     @IsOptional()
     @IsAlpha()
     sortBy: string;
 
     @IsOptional()
     @IsEnum(Roles)
     role: string = Roles.User;
 
     @IsOptional()
     @IsBoolean()
     showAll: boolean = true;
 
 }

describe("@QueryParam should use default values from query descriptor when it's not exist on the requets", () => {
        assertRequest([3001, 3002], "get", "photos?sortBy=name&showAll", response => {
            expect(queryParamSortBy).to.be.equal("name");
            expect(queryParamLimit).to.be.equal(30);
            expect(queryParamShowAll).to.be.equal(null);
            expect(response).to.be.status(200);
            expect(response).to.have.header("content-type", "text/html; charset=utf-8");
        });
    });

MichalLytek · 2017-09-01T08:33:28Z

Also this current approach doesn't allow partial description of the queryparams because of the incorrect behaviour of IsOptional decorator.

It allows just like it allowed before @IsOptional was introduced - just use skipMissingProperties and @IsDefined 😉

For this reason I would return null instead of undefined.

No, keep consistent with TypeScript, where optional fields are Type | undefined.

Another idea could be to support default params?

As you can see this can be easily done in userland, no need for special treatment on framework level.
However I'm not sure if it works well with class-transformer.

MichalLytek · 2017-09-03T10:06:26Z

As you can see this can be easily done in userland, no need for special treatment on framework level.
However I'm not sure if it works well with class-transformer.

Ok, looks like it works - you can define defaults on class level 😉

import { plainToClass } from "class-transformer"

class Framework {
    name: string = "type-express"
    version: number = 0.72
    isProductionReady: boolean
}

const test = plainToClass(Framework, {
    isProductionReady: false,
})

console.log(test) // Framework { name: 'type-express', version: 0.72, isProductionReady: false }

I've added a test case for optional query params with defaults and it works great! You can merge this PR now @NoNameProvided, I will do another one with enhanced params normalization feature soon - this one only brings back the @QueryParams decorator options 😉

NoNameProvided · 2017-09-05T06:25:42Z

No, keep consistent with TypeScript, where optional fields are Type | undefined.

I am still not agree with this. My problem is still that ?paramA=3&paramB should not be equal to ?paramA=3 and if we default to undefined it will, because there is no way to tell if paramB exists without a value or doesn't exists at all, and its an important information.

So I would like to see:

?paramA=3&paramB=5 --> { paramA: 3, paramB: 5 }
?paramA=3&paramB --> { paramA: 3, paramB: null }
?paramA=3 --> { paramA: 3 } (aka paramB is undefined)

When a parameter is there with no value it's means exactly the same as in JS with a variable without value (aka null).

MichalLytek · 2017-09-05T11:34:42Z

Variable without value is undefined, not null. Try:

let x;
console.log("x", x);

let y = 5;
delete y;
console.log("y", y);

because there is no way to tell if paramB exists without a value or doesn't exists at all, and its an important information

But your controller want to receive name param from query string. What is the difference if your receive undefined when param not present and null when param is invalid no string value?
GET /user?name and GET /user
In both cases you have no value so you return all values from db, not filtered. Null is bad in JS/TS and we shoudn't use it unless it's necessary (TypeORM deletes fields when value is null, not undefined to allow partial entity updates).

By default, node.js querystring module parse /user?name to { name: "" }. I won't normalize empty string to null value for string param demand, I can only throw NormalizeError. The discussion is here:
#234 (comment)

pleerock · 2017-09-05T12:15:49Z

How express reacts if we pass ?paramA=3&paramB ? What is value of paramB in req.query ? is it null or undefined? I think we should do same as what they do

MichalLytek · 2017-09-05T12:22:19Z

It is empty string.
https://repl.it/KhlJ

pleerock · 2017-09-05T12:27:01Z

Ah okay it means we cant make decision based on it for numbers, boolean, etc. Im not sure what is correct null or undefined. For strings it probably must stay "", but for others... @NoNameProvided is there any cases why you need null instead of undefined ?

MichalLytek · 2017-09-05T12:33:10Z

Both ?paramA=&paramB produces empty string that we have to normalize for boolean to true as its common to do e.g. /todos?showUncompleted.

pleerock · 2017-09-06T04:42:51Z

Im not sure about such feature. Its very seamless feature

MichalLytek · 2017-09-06T09:29:55Z

Ok, but as I said - normalizing query object will be part of another PR. This one only bring back the query params options to let control validation and other options. Could you merge it?

NoNameProvided · 2017-09-08T07:29:17Z

It is empty string.
https://repl.it/KhlJ

Express doesn't use the querystring package. Or does it? I cannot find it in their package.json.

Im not sure what is correct null or undefined. For strings it probably must stay "", but for others...

I dont get it why you guys want empty string.

@NoNameProvided is there any cases why you need null instead of undefined ?

I want to make an easily usable difference between sent params with no value and not sent params. When I send a param I do it on a purpose, so I want it to have a different value than undefined which represent uninitialized values.

I get @19majkel94 reasoning that variables without value are undefined, but we should approach this from the side of reading query params not from the side of js.

Lets take an example: I have a searchTerm param.

What I am interested in is that if it's has meaningful value. With your proposal I would have to do if(searchTerm && searchTerm.length > 0) with my proposal I have to do if(searchTerm) because only meaningful values are handled as values.

If I am interested in seeing if the param was sent without value I can do if(searchTerm == null) what explains better for me than if(searchTerm == "")

Both ?paramA=&paramB produces empty string that we have to normalize for boolean to true as its common to do e.g. /todos?showUncompleted.

I am ok with this. It's a nice trick, just needs to be outlined in the docs.

MichalLytek · 2017-09-08T08:10:29Z

Express doesn't use the querystring package. Or does it? I cannot find it in their package.json.

It's a built-in node module. And they use "qs" package which works in the same way:

I want to make an easily usable difference between sent params with no value and not sent params. When I send a param I do it on a purpose, so I want it to have a different value than undefined which represent uninitialized values.

As you see in node.js world we distinguish empty query value and no query value (empty string and undefined). Our task is just to normalize the params like converting "true" to true or "123" to 123. So as both ?paramA=&paramB produces empty string, we should map the empty string to true for boolean, for number throw normalizing error and leave it as it is for string params. No undefined, no null, that's it.

What I am interested in is that if it's has meaningful value. With your proposal I would have to do if(searchTerm && searchTerm.length > 0) with my proposal I have to do if(searchTerm) because only meaningful values are handled as values.

Have you forgotten that "" and 0 evaluates to false?

const str = ""
if (str) {
    console.log("empty string?")
} else {
    console.log("no, conversion to truthy values happen") // logs this
}

NoNameProvided · 2017-09-08T10:08:06Z

Have you forgotten that "" and 0 evaluates to false?

Damn, forgotten about "" 😱

Yet I am not convienced on the string one. But it's 2 vs 1 so I guess we will go that way.

MichalLytek · 2017-09-10T13:33:19Z

If I am interested in seeing if the param was sent without value I can do if(searchTerm == null) what explains better for me than if(searchTerm == "")

But what business logic depends if the string param has been set but with no value. Like:

GET /users?city

if (city === undefined) { // city param not exist
   return usersService.getAll();
} else if (city === null) { // city param exist but has no value
   return usersService.getUsersWithCityDefined(); // ???
}

For boolean no value means true, not exist means undefined, right?
For numbers no value means no number, so undefined. If it's required we have then ParamRequiredError.
So as I explained, for string we signalize the "exist but with empty value" by empty string. No need to pollute by setting value to null which breaks TS type system.

MichalLytek · 2017-09-11T17:32:12Z

Closing in favor of #289.

github-actions · 2020-12-12T00:55:05Z

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.

MichalLytek added 3 commits August 31, 2017 19:28

Fix missing @queryParams options

e8cbf59

Add normalizing feature to queries object

3e71052

Create test cases for @queryParams and its options

1c97969

MichalLytek added type: fix Issues describing a broken feature. enhancement labels Aug 31, 2017

MichalLytek requested review from pleerock and NoNameProvided August 31, 2017 17:31

MichalLytek commented Aug 31, 2017

View reviewed changes

MichalLytek added 2 commits August 31, 2017 19:51

Add check if property type is not undefined (no type annotation)

6353ca1

Add info in docs about @queryParams() features

4eac44c

MichalLytek commented Aug 31, 2017

View reviewed changes

NoNameProvided requested changes Aug 31, 2017

View reviewed changes

MichalLytek mentioned this pull request Sep 1, 2017

@QueryParam enum support #208

Closed

Add optional query params test case

1ca0c3c

MichalLytek removed the request for review from pleerock September 3, 2017 10:07

MichalLytek self-assigned this Sep 3, 2017

MichalLytek mentioned this pull request Sep 4, 2017

QueryParam() doesn't throw error, when trying to cast non numerical value into number #234

Closed

Merge branch 'master' into fix/params-decorator-options

5aa5087

MichalLytek closed this Sep 11, 2017

MichalLytek deleted the fix/params-decorator-options branch September 16, 2017 09:40

github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix missing @QueryParams decorator options #270

Fix missing @QueryParams decorator options #270

MichalLytek commented Aug 31, 2017 •

edited

Loading

MichalLytek Aug 31, 2017 •

edited

Loading

MichalLytek Aug 31, 2017

NoNameProvided Aug 31, 2017

MichalLytek Sep 1, 2017

NoNameProvided commented Aug 31, 2017 •

edited

Loading

NoNameProvided commented Aug 31, 2017

MichalLytek commented Sep 1, 2017 •

edited

Loading

MichalLytek commented Sep 3, 2017

NoNameProvided commented Sep 5, 2017 •

edited

Loading

MichalLytek commented Sep 5, 2017 •

edited

Loading

pleerock commented Sep 5, 2017

MichalLytek commented Sep 5, 2017

pleerock commented Sep 5, 2017

MichalLytek commented Sep 5, 2017

pleerock commented Sep 6, 2017

MichalLytek commented Sep 6, 2017

NoNameProvided commented Sep 8, 2017

MichalLytek commented Sep 8, 2017 •

edited

Loading

NoNameProvided commented Sep 8, 2017

MichalLytek commented Sep 10, 2017

MichalLytek commented Sep 11, 2017

github-actions bot commented Dec 12, 2020

Fix missing @QueryParams decorator options #270

Fix missing @QueryParams decorator options #270

Conversation

MichalLytek commented Aug 31, 2017 • edited Loading

MichalLytek Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

MichalLytek Aug 31, 2017

Choose a reason for hiding this comment

NoNameProvided Aug 31, 2017

Choose a reason for hiding this comment

MichalLytek Sep 1, 2017

Choose a reason for hiding this comment

NoNameProvided commented Aug 31, 2017 • edited Loading

NoNameProvided commented Aug 31, 2017

MichalLytek commented Sep 1, 2017 • edited Loading

MichalLytek commented Sep 3, 2017

NoNameProvided commented Sep 5, 2017 • edited Loading

MichalLytek commented Sep 5, 2017 • edited Loading

pleerock commented Sep 5, 2017

MichalLytek commented Sep 5, 2017

pleerock commented Sep 5, 2017

MichalLytek commented Sep 5, 2017

pleerock commented Sep 6, 2017

MichalLytek commented Sep 6, 2017

NoNameProvided commented Sep 8, 2017

MichalLytek commented Sep 8, 2017 • edited Loading

NoNameProvided commented Sep 8, 2017

MichalLytek commented Sep 10, 2017

MichalLytek commented Sep 11, 2017

github-actions bot commented Dec 12, 2020

MichalLytek commented Aug 31, 2017 •

edited

Loading

MichalLytek Aug 31, 2017 •

edited

Loading

NoNameProvided commented Aug 31, 2017 •

edited

Loading

MichalLytek commented Sep 1, 2017 •

edited

Loading

NoNameProvided commented Sep 5, 2017 •

edited

Loading

MichalLytek commented Sep 5, 2017 •

edited

Loading

MichalLytek commented Sep 8, 2017 •

edited

Loading