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

Define a required parameter using a sugar API #1940

Open
4 tasks
bajtos opened this issue Oct 30, 2018 · 8 comments
Open
4 tasks

Define a required parameter using a sugar API #1940

bajtos opened this issue Oct 30, 2018 · 8 comments
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue Hacktoberfest Tasks ready for new contributors to work on REST Issues related to @loopback/rest package and REST transport in general

Comments

@bajtos
Copy link
Member

bajtos commented Oct 30, 2018

At the moment, it is not possible to use @param shortcuts like @param.query.string to define a required parameter.

One has to use @param directly (but don't have to specify the type since LB4 can infer the type from TypeScript metadata). For example:

@param({name: 'format', in: 'query', required: true}) format?: string

Ideally, I would like LB4 to provide the following syntax for annotating parameters as required:

@param.query.string('format', {required: true}) format?: string

Acceptance criteria

  • Modify all @param.SOURCE.TYPE() shortcuts (and possibly other similar shortucts) to accept a new optional argument of type Partial<ParameterObject> and use the provided properties to enhance (or amend) the spec generated by the decorator.
  • Modify @param.array() in a similar way, the new optional argument will be the fourth arg.
  • Modify @param.query.object() similarly, the new optional argument will be the third arg.
  • Verify that the API documentation is able to pick up the changes

🎆 Hacktoberfest 2020

Greetings 👋 to all Hacktoberfest 2020 participants!

Here are few tips 👀 to make your start easier, see also #6456:

  • Before you start working on this issue, please leave a comment to let others know.
  • If you are new to GitHub pull requests, then you can learn about the process in Submitting a pull request to LoopBack 4.
  • If this is your first contribution to LoopBack, then please take a look at our Developer guide
  • Feel free to ask for help in #loopback-contributors channel, you can join our Slack workspace here.
@bajtos bajtos added developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue REST Issues related to @loopback/rest package and REST transport in general labels Oct 30, 2018
@bajtos bajtos added the Hacktoberfest Tasks ready for new contributors to work on label Oct 2, 2020
@mdbetancourt
Copy link
Contributor

i think

@param.query.required() format: string

it's very cool isn't?

@bajtos
Copy link
Member Author

bajtos commented Oct 6, 2020

@mdbetancourt Your proposal looks nice. Please keep in mind that the parameter name cannot be inferred at runtime, it must be provided by the developer.

@param.query.required('format') format: string

Often the developer needs to provide the type too, so we will end up with quite few .required() functions to implement!

@param.path.number.required('id') id: number

In general, there are more ParameterObject properties that the user may want to set in addition to required. The proposal described in the issue is generic enough to support those use cases too. I am fine to implement your proposal, it does provide a nicer solution for required parameters in particular 👍🏻 I just think we will eventually have to implement a more generic solution anyways, so I am not sure if a special API for required parameters is worth the effort 🤷🏻 I'll leave it up to you to decide 😄

@mdbetancourt
Copy link
Contributor

mdbetancourt commented Oct 9, 2020

Please keep in mind that the parameter name cannot be inferred at runtime, it must be provided by the developer.

I had certainly forgotten 😄

since required is a common use case we can use

@param.query.require.string('format')

or

@param.require.query.string('format')

feels natural and similar to chai style (which we are already using for tests) and of course we should try to keep it simple (at least for builtin type which can be inferred)

@param.require.query('format')
format: string
// for complex types
@param.require.type(Type).query('format')
format: Type
// arrays
@param.require.array.type(Type).query('format')
format: Type[]
// or
@param.require.string.array.query('format')
format: Type[]

what do you think? maybe too large?

@bajtos
Copy link
Member Author

bajtos commented Oct 9, 2020

I don't have a strong opinion. I guess it depends on how much time you want to invest into building a chain-style API? I think it would be best to start with adding a new argument accepting Partial<ParameterObject>, to provide a short-term solution. Then you can look into ways how to make the API even more easier to use, e.g. by providing a chained API.

I feel it's important to preserve backwards compatibility here, so even if we implement @param.require.query('format'), then the existing flavor @param.query('format') must keep working.

@raymondfeng
Copy link
Contributor

I'm not a big fan to have a very long decorator. It would be better to add an optional argument for the sugar decorators to accept additional information, such as:

@param.query.string('access_token', {required: true})

The 2nd argument should be typed as Partial<Omit<ParameterObject, 'type' | 'in'>> to avoid overriding type and in.

@MattiaPrimavera
Copy link
Contributor

I'm interested in contributing to this one!

  • should an error be raised in case the required parameter is not found ?
  • are we going with @param.query.string('access_token', {required: true}) ?

@bajtos
Copy link
Member Author

bajtos commented Mar 12, 2021

@MattiaPrimavera

I'm interested in contributing to this one!

Cool! Sorry for the long silence on our side 🙈

  • should an error be raised in case the required parameter is not found ?

I believe that's already handled by our REST validation layer. This GH issue is about adding a shorter way how to define required parameters in the code.

  • are we going with @param.query.string('access_token', {required: true}) ?

Yes please 👍

@MattiaPrimavera
Copy link
Contributor

Cool! Sorry for the long silence on our side 🙈

Hello @bajtos, no worries, and sorry it took me a while to answer too :)

I believe that's already handled by our REST validation layer. This GH issue is about adding a shorter way how to define required parameters in the code.

Ok thanks

Yes please 👍

Can I still take a look at this one, if still available ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Issues affecting ease of use and overall experience of LB users feature good first issue Hacktoberfest Tasks ready for new contributors to work on REST Issues related to @loopback/rest package and REST transport in general
Projects
None yet
Development

No branches or pull requests

4 participants