feathersjs-ecosystem / feathers-swagger

Add documentation to your FeatherJS services and feed them to Swagger UI.
MIT License
226 stars 63 forks source link

Is support for pagination and 'multi' create needed? #218

Closed toddbaert closed 1 year ago

toddbaert commented 3 years ago

I'd love to create a PR to support pagination: https://docs.feathersjs.com/api/databases/common.html#pagination

As well as to add support for 'multi' create: https://docs.feathersjs.com/api/services.html#create-data-params

If I'm not missing anything, these need to be added. Can I go ahead, and if so, will I get the required attention to merge the code?

Mairu commented 3 years ago

Hi,

I also thought about adding support for pagination out of the box for the next version. But it would be a breaking change if not hidden behind an option.

You can already achieve it by customizing the defaults.operationsGenerator for find. So the first step would be to create that, this could be a documentation or FAQ entry for the current version and then used for the next version.

For the create multi I intentionally added no support, because in comparison to the other multi operations it is exactly the same endpoint as create. So you can only change the requestBody and response of the current create operation to show arrays. At the time I implemented it, there was no real swagger support for supporting multiple types. And at least swagger-ui does not support it well enough I think to have both single and multi creation documented at the same time. Theoretically, it would be the usage of https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/, also this can be changed by using a defaults.operationsGenerator for create.

I am open for any thoughts.

toddbaert commented 3 years ago

Wow, thanks for the great comment. I agree that this could be accomplished mostly with doc/examples, and that's a better option for this verion. I think both would be helpful, especially since pagination is supported out of the box with feathers. If you think it's valuable, instead of implementing anything in the lib, I will simply add some doc and examples around pagination (and multi-create as well if I can find a decent solution). Does that sound OK to you?

EDIT:

Looking into it more, I'm not so satisfied with the operationsGenerator implementation. The problem I personally see is, that function doesn't have a model name as input, which TbH seems like the best way to implement this.

Wouldn't it be fairly simple and non-breaking to add a boolean "paginated" property to ServiceSwaggerOptions (similar to the existing multi) and in the default implementation of the schema list generation, make the list paginated instead:

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/81dd8a3e17438ddbb4276d8199e07137fcc561b4/lib/v3/generator.js#L4

Then users could simply add paginated: true and get this nice behaviour.

Mairu commented 3 years ago

The line you mentioned is not the one for the operationsGenerator.

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/types/index.d.ts#L56-L68

are the available options you can consume in a custom operations generator.

The relevant code:

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/lib/openapi.js#L148

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/lib/openapi.js#L65-L91

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/lib/openapi.js#L155

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/lib/openapi.js#L175-L184

and

https://github.com/feathersjs-ecosystem/feathers-swagger/blob/179fa50856c785e9984ae02847bae3d083fc3ca0/lib/openapi.js#L205

So there is really all information you can rely on available I would say.

For pagination, you are right that such an option could be added, but my plan was to use the service.options directly, so that no extra option would be needed. This can also be read in a operationsGenerator.

Edit: And of course it would be fine to add examples and documentation. Also PRs for the planned changes would be welcome.

toddbaert commented 3 years ago

The line you mentioned is not the one for the operationsGenerator.

I understand that. What I was trying to say is we could easily add a default pagination implementation in that code... but I like your idea to do it in the operationsGenerator better. I had missed that we had the required params there.

For pagination, you are right that such an option could be added, but my plan was to use the service.options directly, so that no extra option would be needed. This can also be read in a operationsGenerator.

Oh, you're absolutely right. So I guess this is what you meant by breaking change... because if we read the actual options here, suddenly pagination would start happening for people in the swagger without them taking any action other than updating. I see how that could be an issue.

I will be implementing basically what you describe for pagination in my own service. Once that's done, I will make a PR here since I think the code will be somewhat similar (changing the default service generator). Are you saying you'd be interested in that even though it might constitute a breaking change?

Mairu commented 3 years ago

Yeah, I would be interested.

Sadly it could take some time to land in a version 2.0

Mairu commented 1 year ago

Support has been added.