Skip to content

Fix LFClassic api 500 error when querying for all entries#2242

Open
imnasnainaec wants to merge 1 commit intodevelopfrom
bugfix/lfclassic-query-all-500
Open

Fix LFClassic api 500 error when querying for all entries#2242
imnasnainaec wants to merge 1 commit intodevelopfrom
bugfix/lfclassic-query-all-500

Conversation

@imnasnainaec
Copy link
Copy Markdown
Collaborator

count=-1 is how to indicate to the API to get all entries, but that is accomplished with MongoDB via .Limit(0), not .Limit(-1).

@imnasnainaec imnasnainaec self-assigned this Apr 10, 2026
@imnasnainaec imnasnainaec added bug Something isn't working 🟥High High-priority PR: please review this asap! labels Apr 10, 2026
@github-actions github-actions bot added the 📦 Lexbox issues related to any server side code, fw-headless included label Apr 10, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 10, 2026

📝 Walkthrough

Walkthrough

Modified MongoDB query pagination behavior in the Query(...) method of LfClassicMiniLcmApi.cs. When options.Count equals QueryOptions.QueryAll, the .Limit(...) value is now set to 0 instead of passing options.Count through unchanged, altering the result set behavior for "query all" requests.

Changes

Cohort / File(s) Summary
MongoDB Query Pagination
backend/LfClassicData/LfClassicMiniLcmApi.cs
Modified pagination logic: .Limit(...) now resolves to 0 when QueryOptions.QueryAll is requested, changing result set behavior for unbounded queries.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

A rabbit hops through query lands so vast,
Where limits once were bound, now fast
To zero they shall flow, when all is sought,
The MongoDB dance, so cleverly wrought! 🐰✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing a 500 error that occurs when querying for all entries in the LFClassic API.
Description check ✅ Passed The description directly explains the bug and its fix, relating precisely to the changeset which corrects the MongoDB Limit value mapping.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch bugfix/lfclassic-query-all-500

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@imnasnainaec imnasnainaec added 🟨Medium Medium-priority PR and removed 🟥High High-priority PR: please review this asap! labels Apr 10, 2026
@github-actions
Copy link
Copy Markdown
Contributor

C# Unit Tests

165 tests  ±0   165 ✅ ±0   18s ⏱️ ±0s
 23 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit 7e0d1f3. ± Comparison against base commit 73e4ecd.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@backend/LfClassicData/LfClassicMiniLcmApi.cs`:
- Around line 213-214: Add input validation in the Query method to reject any
options.Count that is negative and not equal to QueryOptions.QueryAll;
specifically check options.Count and if (options.Count < 0 && options.Count !=
QueryOptions.QueryAll) return a client error (e.g., BadRequest / throw
ArgumentException) rather than letting the value reach the .Limit(...) call,
ensuring only the sentinel -1 is allowed and all other negative counts are
rejected.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c1800774-7d2d-401e-8fb6-f2267b6e3d2d

📥 Commits

Reviewing files that changed from the base of the PR and between 73e4ecd and 7e0d1f3.

📒 Files selected for processing (1)
  • backend/LfClassicData/LfClassicMiniLcmApi.cs

Comment on lines 213 to +214
.Skip(options.Offset)
.Limit(options.Count)
.Limit(options.Count == QueryOptions.QueryAll ? 0 : options.Count)
Copy link
Copy Markdown

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "=== QueryOptions definitions / count validation ==="
fd -i 'QueryOptions' backend -x sed -n '1,220p' {}

echo
echo "=== LFClassic count binding and limit call sites ==="
rg -n -C3 --glob 'backend/**/*.cs' 'QueryAll|\.Limit\s*\(|\bCount\b|\bcount\b|ArgumentOutOfRangeException'

Repository: sillsdev/languageforge-lexbox

Length of output: 50386


🏁 Script executed:

# Check if QueryOptions.Count has validation attributes
rg -A 5 'record QueryOptions|int Count' backend/FwLite/MiniLcm/IMiniLcmReadApi.cs

# Check if LfClassicRoutes.Count has validation
rg -B 2 -A 5 'public int\? Count' backend/LfClassicData/LfClassicRoutes.cs

# Check for any validation on negative counts in the Query method or upstream
rg -B 3 -A 8 'public.*Query.*QueryOptions' backend/LfClassicData/LfClassicMiniLcmApi.cs | head -40

Repository: sillsdev/languageforge-lexbox

Length of output: 533


🏁 Script executed:

# Check for any ArgumentException or validation in Query method implementation
rg -B 5 -A 25 'private.*IAsyncEnumerable.*Query.*QueryOptions' backend/LfClassicData/LfClassicMiniLcmApi.cs

# Check if there's any validation in the route handler or controller
rg -B 3 -A 10 'Count.*QueryAll|ArgumentOutOfRangeException.*Count|ArgumentException.*Count' backend/LfClassicData/

Repository: sillsdev/languageforge-lexbox

Length of output: 3551


Add validation to reject non-sentinel negative counts.

This fixes the QueryOptions.QueryAll case (count=-1), but options.Count < -1 still reaches .Limit(...). There is no upstream validation on the Count parameter in LfClassicRoutes or within the Query method itself. If a client sends count=-2, it will pass the conditional check and cause MongoDB to reject the aggregation pipeline with a 500 error, reproducing the original issue.

Add a guard to reject every negative value except QueryAll:

Suggested fix
+        var limit = options.Count switch
+        {
+            QueryOptions.QueryAll => 0,
+            < 0 => throw new ArgumentOutOfRangeException(nameof(options.Count), "Count must be >= 0 or QueryAll."),
+            _ => options.Count
+        };
+
         pipeline = AppendHeadWord(sortWs, pipeline)
             .Sort(Builders<HeadwordEntry>.Sort
                 .Ascending(entry => entry.headword)
                 .Ascending(entry => entry.MorphologyType)
                 .Ascending(entry => entry.Guid))
             .Skip(options.Offset)
-            .Limit(options.Count == QueryOptions.QueryAll ? 0 : options.Count)
+            .Limit(limit)
             .Project(entry => entry as Entities.Entry);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@backend/LfClassicData/LfClassicMiniLcmApi.cs` around lines 213 - 214, Add
input validation in the Query method to reject any options.Count that is
negative and not equal to QueryOptions.QueryAll; specifically check
options.Count and if (options.Count < 0 && options.Count !=
QueryOptions.QueryAll) return a client error (e.g., BadRequest / throw
ArgumentException) rather than letting the value reach the .Limit(...) call,
ensuring only the sentinel -1 is allowed and all other negative counts are
rejected.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myieye Would you prefer if I also add this check for invalid negative counts?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working 📦 Lexbox issues related to any server side code, fw-headless included 🟨Medium Medium-priority PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant