Limit does not have an upper bound

[yamcodes]

Describe the bug

The plugin caller can specify an arbitrary number for the limit. We should probably enforce a maximum limit to keep performance in check.

Reproduction

Call any queriable plugin (route) and use a huge limit. You'll lose the benefit of pagination, hurting performance.

System Info

No response

Validations

• Read the Contributing Guidelines.
• Check that there isn't already an issue that reports the same bug.
• Check that this is a concrete bug. For Q&A, open a GitHub Discussion.
• The provided reproduction is a minimal reproducible example of the bug.

[yamcodes]

Continued from #134

@coderabbitai @Hajbo Thoughts about this ? Do we need this ?

What about the offeset do we need to set any upper bound on it (That one doesn't make sense lol)

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

I'll need to gather more context about the current implementation to give a proper opinion on this. Let me look into how pagination is currently implemented in the codebase.

---

🏁 Script executed:

#!/bin/bash
# First, let's check the PR that was mentioned to understand the context
gh pr view 134 -c

# Let's look for files that might contain pagination logic with limit and offset
rg -i "limit|offset" --type js --type ts -A 5 -B 5

# Let's specifically look for any validation related to limit or offset
rg -i "validate.*limit|check.*limit|max.*limit" --type js --type ts

Length of output: 94024

---

Let me search for more information about the pagination implementation to understand how limits and offsets are currently handled in the codebase.

---

🏁 Script executed:

#!/bin/bash
# Search for pagination-related code, focusing on schema definitions and limit/offset usage
rg -i "pagination|limit|offset" --type ts

# Look specifically at schema files which might define query parameters
find . -name "*.schema.ts" -exec grep -l -i "limit\|offset" {} \;

# Check for any existing validation of query parameters
rg -i "validate.*query|check.*query" --type ts

Length of output: 1453

---

Based on the search results, I can see that pagination is implemented in the articles module using limit and offset parameters. Here's what's currently in place:

1. In src/articles/articles.schema.ts, the schema defines:

   limit: Type.Optional(Type.Number({ minimum: 1, default: 20 })),
   offset: Type.Optional(Type.Number({ minimum: 0, default: 0 })),

2. The schema ensures:

   • limit has a minimum value of 1 with a default of 20
   • offset has a minimum value of 0 with a default of 0

3. However, there's no maximum value set for limit, which could lead to performance issues if someone requests a very large number of results.

@yamcodes For pagination limits, I do think adding an upper bound on the limit parameter would be beneficial:

1. It would prevent performance degradation from excessive data retrieval
2. It would maintain the intended behavior of pagination
3. It would protect both the server and database from resource exhaustion

A common practice is to set the maximum limit between 100-1000 depending on your application's needs.

The implementation would be simple, just add a maximum property in the schema:

limit: Type.Optional(Type.Number({ minimum: 1, maximum: 100, default: 20 })),

For the offset parameter, I don't think an upper bound is necessary since extremely large offsets would just return empty results, though they might cause inefficient query planning in some databases. The primary concern is the limit parameter.

Would you like me to suggest a specific maximum value for your use case?

[yamcodes]

@coderabbitai should probably just use an environment variable for that no ?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for environment variable usage patterns in the codebase
rg -i "process.env" --type ts

# Look for configuration files that might handle environment variables
find . -name "*.config.ts" -o -name "*.env*" -o -name "config*.ts"

# Check if there's any existing maximum limit implementations
rg -i "maximum.*limit|max.*limit" --type ts

# Look for schema files to understand how pagination is implemented
find . -name "*.schema.ts" | xargs grep -l "limit"

Length of output: 625

---

🏁 Script executed:

#!/bin/bash
# Check the content of the articles schema to see current pagination implementation
cat ./src/articles/articles.schema.ts

# Check the main config file to understand how environment variables are used
cat ./src/config.ts

# Check .env file for any existing pagination-related variables
cat ./.env

# See if there are any other schema files using pagination
rg -i "limit.*type.number|type.number.*limit" --type ts

Length of output: 3725

---

@yamcodes Yes, using an environment variable is definitely the way to go! This would allow you to configure the limit based on your environment needs without code changes.

Here's how you could implement it:

1. Add a new variable to your .env file:

MAX_PAGINATION_LIMIT=100

2. Update your src/config.ts to include this new variable in the schema:

const envSchema = Type.Object({
  // existing variables...
  MAX_PAGINATION_LIMIT: Type.String(),
});

3. Then use it in your schema definition:

limit: Type.Optional(Type.Number({ 
  minimum: 1, 
  maximum: Number(env.MAX_PAGINATION_LIMIT), 
  default: 20 
})),

This approach gives you flexibility to adjust the limit based on your server's capacity without redeploying code.

[Hajbo]

The spec doesn't say anything about it, but we can set it to something. Since the article body is not returned and the returned articles are small, it could easily be a 1000.