Take limit in ListOptions

[hendrik-advantitge]

Is your feature request related to a problem? Please describe.
When running a Vendure instance that contains many Products and ProductVariants, it is currently possible to query all products at once as there is no limit for the take input. This is obviously not desired as this can easily bring down the server.

Describe the solution you'd like
Allow to define a limit and default value for the take variable in ListOptions and SearchInput.

Describe alternatives you've considered

• Define a Custom Scalar with a maximum and default value.
• Other options?

[michaelbromley]

I agree that we need this. I'm not sure how it should be configured. Some ideas:

• Add a listItemsLimit: number property to apiOptions. Read it in the ListQueryBuilder service and apply a Math.min() operation to the take option. Do we apply the same for the Admin & Shop APIs? I guess it would be ok to do so.
• Use some kind of middleware, e.g. an ApolloServerPlugin which looks for input types which match the <entity>ListOptions name/shape and imposes a limit on the take variable.
• Custom scalar would be a breaking change I guess. Also how do you configure a custom scalar? I've not done anything like that so far.
• Maybe even allow the user to define a ListQueryBuilderStrategy which allows you to intercept calls to ListQueryBuilder.build() and apply some arbitrary logic, returning a ListQueryOptions object. Then supply a DefaultListQueryBuilderStrategy({ takeLimit: number }). This might be nice for other more complex use-cases too.

[hendrik-advantitge]

As a general remark, we would like to be able to distinguish between the Admin API and the Shop API. We would like to set the limit on the Shop API a lot lower.

• Option 1: seems straightforward, but might not be that obvious from an API user perspective
• Option 2: would work, but seems rather complicated. Especially when introducing API components that differ from the ListOptions (such as SearchInput).
• Option 3: custom scalar is not a breaking change. It is pretty easy to implement (https://docs.nestjs.com/graphql/scalars), and also easy to use in plugins.
• Option 4: allows for a lot of flexibility, but I'm asking myself the question if that is actually useful. I can not seem to think of many use cases.

In conclusion, my preference goes out to Option 2.
A totally different approach would be to use a package like https://github.com/slicknode/graphql-query-complexity.
However, nothing suggests both solutions could not coexist.

[michaelbromley]

If defining a custom scalar, the question remains of how to make it configurable? Would you provide your own with an identical name to the built-in one to override it?

[hendrik-advantitge]

I might not be familiar enough with all concepts of NestJS, but Custom Scalars have to be included in the Module as a provider, so I suppose they support dependency injection? In that case the config can be read.

[michaelbromley]

In that case the config can be read.

If we are going to be reading from the ConfigService anyway, I think the best & simplest method then is to inject the ConfigService into the ListQueryBuilder and read the max value in build() method (option 1 in my original comment). You mentioned "but might not be that obvious from an API user perspective" - can you expand on that?

[hendrik-advantitge]

What I meant by that is that the API would still expose an Int scalar, but in the background a limit is put on this value.

• If we return an error when the limit on take is exceeded, the user knows what is happening.
• If we decide to just cap the value in the background, the user might not know what is going on and think the take option is somehow 'broken'.

However I feel that I am really nitpicking at this point. What matters most for us is that the limit on the shop and admin API can be set differently.

[michaelbromley]

Got it, thanks for the explanation. One option to make the limit more explicit would be to throw a UserInputError when attempting to take too many.

[alex-dwt]

Hello @michaelbromley
May be I don't understand something, but I've just checked this feature, and for queries products{ } and collections{ } it works.
But for search{ } it doesn't work, I can take any count of items and the restriction doesn't work.

Is it okay? Why? How can I set restriction for search{ } also?
Thanks.

[michaelbromley]

Hi @alex-dwt. This is because the search query uses a different mechanism for retrieving results. Internally, the other list queries like collections products etc use a class named ListQueryBuilder to generate the DB query to fetch results. These queries can involve numerous DB joins and can get rather expensive performance-wise. The search query is different because it uses a special de-normalized index designed for efficiency. The query does not use DB joins and as a result the risk from large queries is minor compared to the other types of list query.

[alex-dwt]

Thanks for a full description @michaelbromley
Initially I've read the text in the 1st message:

Describe the solution you'd like
Allow to define a limit and default value for the take variable in ListOptions and SearchInput.

And because of SearchInput I decided that you are going to improve search_query also, and start thinking that you probably missed this -)
PS. But maybe it makes some sense to "protect" search_query also, or maybe not, anyway I agree it's not so important now