loopback-next icon indicating copy to clipboard operation
loopback-next copied to clipboard

Define a required parameter using a sugar API

Open bajtos opened this issue 5 years ago • 8 comments

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 :wave: 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 avatar Oct 30 '18 10:10 bajtos

i think

@param.query.required() format: string

it's very cool isn't?

mdbetancourt avatar Oct 02 '20 19:10 mdbetancourt

@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 😄

bajtos avatar Oct 06 '20 06:10 bajtos

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

I had certainly forgotten :smile:

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?

mdbetancourt avatar Oct 09 '20 03:10 mdbetancourt

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.

bajtos avatar Oct 09 '20 14:10 bajtos

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.

raymondfeng avatar Oct 09 '20 15:10 raymondfeng

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}) ?

MattiaPrimavera avatar Oct 20 '20 10:10 MattiaPrimavera

@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 :+1:

bajtos avatar Mar 12 '21 16:03 bajtos

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 ?

MattiaPrimavera avatar Oct 10 '21 21:10 MattiaPrimavera