Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new Queries decorator to include all query parameters in one sing… #1309

Merged
merged 6 commits into from
Nov 25, 2022
Merged

Add new Queries decorator to include all query parameters in one sing… #1309

merged 6 commits into from
Nov 25, 2022

Conversation

daniseijo
Copy link
Contributor

@daniseijo daniseijo commented Sep 12, 2022

This PR adds a new @Queries decorator that accepts an object and wraps all the query parameters into it. This is something very useful for example if the request has a lot of filter parameters that we want to handle. It has been requested several times for example in this issue.

I have tried to keep this new decorator behavior as similar as the Body and the BodyProps decorators were implemented.

Use example:

export interface QueryParams {
  numberParam: number;
  stringParam: string;
  booleanParam: boolean;
  optionalStringParam?: string;
}


@Get('AllQueriesInOneObject')
public async getAllQueriesInOneObject(@Queries() queryParams: QueryParams) {
  const model = new ModelService().getModel();
  model.optionalString = queryParams.optionalStringParam;
  model.numberValue = queryParams.numberParam;
  model.boolValue = queryParams.booleanParam;
  model.stringValue = queryParams.stringParam;

  return model;
}

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?
  • This PR is associated with an existing issue? (Dynamic Query/Header Param #353)

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach

I have not tested yet if it would be a problem to have both the Queries and the Query decorator in one single function. It would be nice to add some test cases testing this.

Test plan

For what I wanted, the code is working as expected. I have tried to keep this new decorator as similar as you have implemented the Body and the BodyProps decorators. I still want to add more tests to check the doc generation but wanted to check if this approach was the correct one before continuing.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Hello there daniseijo 👋

Thank you and congrats 🎉 for opening your first PR on this project.✨

We will review the following PR soon! 👀

@WoH
Copy link
Collaborator

WoH commented Sep 12, 2022

Looks great!
I'll write down some questions which I am asking myself, maybe some you already considered.

Validation: Seems to make sense, I'll have to check what happens if we combine with @Query() under different excess prop handling scenarios

Spec: No immediate questions, we can add that as another query parameter I presume.
Maybe need to check if it aligns with how express etc. would parse it.

In general I think the easiest way to deal with the combination of several Query and Queries mixes is to not allow it. Otherwise this may blow up in complexity.

There's an issue similar to this which proposed @Query('*') or something similar, but I like the explicit @Queries better.

@daniseijo
Copy link
Contributor Author

Hi! Thanks for the quick response, I would love a bit of help with the swagger part. I have created a method to spread the properties of the Queries object to show its internal query parameters on the swagger document. However, I'm not sure if this is the best approach.

Could you check it out and also help me with the next steps to get this up and ready?

PD.: I will try to add more negative test cases to check all the variations but I think I will not have time to advance until next week.

@rwmblabs
Copy link

rwmblabs commented Oct 4, 2022

Do we have any updates here? I really like this solution 😂

@daniseijo
Copy link
Contributor Author

Do we have any updates here? I really like this solution 😂

Hi, I implemented a solution for the swagger part but I am not sure if there is a cleaner way. If it is ok, maybe @WoH could help me with the next steps and have it merge.

@WoH
Copy link
Collaborator

WoH commented Oct 5, 2022

Thanks for the ping, I'll try to find time over the weekend to leave suggestions if needed

@WoH
Copy link
Collaborator

WoH commented Oct 7, 2022

Can you rebase this PR? CI on master should work again

@@ -458,6 +467,15 @@ export class SpecGenerator3 extends SpecGenerator {
return mediaType;
}

private buildQueriesParameter(source: Tsoa.Parameter): Swagger.Parameter3[] {
if (source.type.dataType === 'refAlias' && source.type.type.dataType === 'nestedObjectLiteral') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

'refObject' should probably work here.
We should not only check 1 level deep on the alias, I've written similar code which checks deeep for isVoidType I think

If we can't handle, I'd rather throw than return an array I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about this, I think is better to fail if there is a nested object, to be coherent with the @Query decorator failing if we pass it a refObject. @WoH What do you think?

packages/cli/src/swagger/specGenerator3.ts Outdated Show resolved Hide resolved
@@ -173,12 +173,22 @@ export class SpecGenerator2 extends SpecGenerator {
pathMethod.security = method.security;
}

const queriesParams: Tsoa.Parameter[] = method.parameters.filter(p => p.in === 'queries');
Copy link
Collaborator

Choose a reason for hiding this comment

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

see spec3

@WoH
Copy link
Collaborator

WoH commented Oct 9, 2022

I think I've mentioned this, but we really don´t want both @Query and @Queries at the same time.
It'll be impossible to manage all that complexity when it comes to what belongs where for now.
For test cases: Please make sure { [name: string]: unknown/any } (basically Records) can be used to wildcard anything.
Maybe { [name: string]: number } and handle that which will be fun.

@github-actions
Copy link

github-actions bot commented Nov 9, 2022

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

@github-actions github-actions bot added the Stale label Nov 9, 2022
@matthewyu-ptr
Copy link

This is a good feature, please don't close this PR

@github-actions github-actions bot removed the Stale label Nov 10, 2022
@aakash-at
Copy link

Any update on this PR?

@WoH
Copy link
Collaborator

WoH commented Nov 16, 2022

Just to be clear, this was not a no from my side. Just throw an Error of both Decorators are parsed in the same method

@daniseijo
Copy link
Contributor Author

Sorry, my intention is to fix the changes but I left my work where I worked with tsoa. Since then I have had no time to continue with this. I will try to take some time to read and solve the issues on the PR as soon as possible. Sorry again for the delay.

@daniseijo
Copy link
Contributor Author

daniseijo commented Nov 24, 2022

Hi, I already made the changes to the code and added some more tests. @WoH, I wrote a response to one of your comments. Check it out and, if you think that behavior does not make sense, tell me and I will change it.

In any case, I think the failing tests in some environments are not my fault anymore and they are more of a problem with a timeout. Whenever you can, check the PR again and let me know if I am missing something.

@WoH WoH merged commit db08b5f into lukeautry:master Nov 25, 2022
@u12206050
Copy link

Awesome! Can't wait for this to be published!

@u12206050
Copy link

@WoH Any chance you could push a new release with this then we can test it? :) Thanks for the work @daniseijo

@filipRisteski
Copy link

I wanted to write something like this, but since it's here it would be very much appreciate if we can speed this up :) Thanks for the hard work @daniseijo

@WoH
Copy link
Collaborator

WoH commented Dec 2, 2022

it's available! yarn add tsoa@next

@bsberry
Copy link

bsberry commented Dec 21, 2022

@daniseijo Thank you so much for implementing this. The lack of this almost stopped our adoption of tsoa cold.

@grug
Copy link

grug commented Dec 23, 2022

Is there any idea of when 5.0.0 will come out of RC and released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants