Repository navigation
ci: fail on new unbounded SQL IN lists and add a Prisma chunking helper - #42629
Conversation
|
|
@greptile re review |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
@greptile re review |
|
how do you deal with the fact that the direct sql function call might have an unbounded in but the place with the actual chunking logic living as an upstream caller ? |
Postgres caps a prepared statement at 32,767 bind parameters and a membership filter binds one per value, so an IN list built from table data breaks once the table outgrows the cap. That is how the budget reset job froze every due budget (LIT-7535, #40564). check_unbounded_in_lists.py reports every Prisma "in" / "not_in" filter whose value has no fixed size and every raw SQL literal that splices a list in after "IN (", unless the line carries "# bounded-ok: <reason>". It only warns for now: the output is the inventory for RCA action item AI-1, and it exits 0.
An ALL_CAPS name imported or filled at runtime is as unbounded as any other, so a name now passes only when the module binds it once to a value of fixed size. Adds Final to the locals a loop does not forbid.
A module list bound once could still grow through append or extend, so a name now counts as fixed only when it is bound to a tuple, frozenset or constant. Trims the module docstring to what a reader needs.
…ded ones Add litellm.repositories.bounded_in: find_many_in, count_in, update_many_in and delete_many_in split a deduplicated value list into 5,000-value chunks, AND each chunk with the caller's where, run them in order (a transaction handle works) and combine the results. Writes take a required atomicity argument, and a where that already filters the chunked field is refused. check_unbounded_in_lists.py now fails CI on any finding missing from unbounded_in_baseline.txt and on any stale baseline entry, so the baseline only shrinks. Entries are keyed by path, enclosing scope, kind, field and occurrence, not line numbers. The helper module is exempt, a constant spread into a frozen tuple counts as fixed, and messages point at the helper for "in" and at an array parameter for "not_in" and raw SQL. A real-Postgres integration test shows a raw 40,000-value filter rejected for too many bind variables while the helpers handle it.
…k size The helper module is litellm.repositories.chunked_in, and its unit and integration tests, the checker's exemption path and its finding messages follow the new name. The `# bounded-ok` marker is unchanged. find_many_in, count_in, update_many_in and delete_many_in take a keyword-only chunk_size, defaulting to IN_LIST_CHUNK_SIZE (5,000). A value below 1 or above MAX_IN_LIST_CHUNK_SIZE (30,000) raises ValueError before any query, which leaves the rest of the filter headroom under Postgres's 32,767 bind-parameter cap.
…_iterable LIT014 (#42650) caps a comprehension at one for and one if clause. The four nested walks in the helper now chain their iterables instead, with the same order and results.
f9a4a26 to
0b2e80a
Compare
| pass | ||
|
|
||
|
|
||
| def _as_clauses(value: object) -> tuple[object, ...]: |
| return (value,) | ||
|
|
||
|
|
||
| def _logical_clauses(clause: object) -> tuple[object, ...]: |
|
bugobt run |
|
@greptileai re review |
…ists _details_for_user_ids reads users through find_many_in instead of a raw "in" filter, so its lookup stays under the bind-parameter cap for any number of recovered keys. Up to 5,000 ids it still sends one find_many with the same where dict, and a PrismaError from any chunk is still logged and treated as no details. The helper now sends each chunk as a list, so a chunked filter equals the dict a hand-written call would send and a migrated call site's existing assertions keep passing. The site's baseline entry is gone.
The dict passed as the field map of TypedDict("Name", {...}), or as its fields= keyword, names fields: an "in" or "notIn" key there is a type, not a filter. Only that dict is skipped, for TypedDict, typing.TypedDict and typing_extensions.TypedDict; a filter nested in a field value or passed to any other call is still reported. The two types/proxy/management_endpoints/team_endpoints.py entries leave the baseline, which is now 156.
Chunks run one after another, so an update that sets the chunked field can move a row into a later chunk, which updates it again and counts it twice: values ["old", "new"] with chunk_size=1 and data={"id": "new"} does exactly that. update_many_in now raises ChunkedFieldWriteError before any query when data has the chunked field as a top-level key, in any form, including Prisma operators such as {"set": ...}.
…and how to clear it It now says what is reported, the three ways to clear a finding, and how the baseline and --update-baseline work, in 11 lines. The per-shape detail lives in the tests.
A baseline key of path, scope, kind, field and occurrence let a PR delete a baselined filter and add a different unbounded one on the same field in the same function, and the new one took over the old key. The key now also carries the filtered expression's source, whitespace-normalized (the Prisma value, or a raw-SQL `IN (...)` slot), so that swap reads as one new and one stale entry and fails the run. The same expression re-added in the same function is still the same finding. Every baseline entry is rewritten in the new form; the 156 findings are unchanged, and only occurrence indexes renumber where one field had several different expressions.
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9c7eae6. Configure here.
TLDR
Problem this solves:
INlist built from table data binds one per value, so it breaks past the capfind_many, and gets take/skip/distinct wrong when it doescount,update_manyanddelete_manyare never splitHow it solves it:
litellm.repositories.chunked_in:find_many_in,count_in,update_many_in,delete_many_inwherechunk_sizeoverrides the 5,000 default; outside 1 to 30,000 it raisesValueErrorbefore any query, leaving the rest of the filter headroom under the capatomicitychoicewherethat already filters the chunked field, and an update whosedatawrites it (a moved row would match a later chunk and update twice)check_unbounded_in_lists.pynow fails CI on any finding not in the baselineunbounded_in_baseline.txt--update-baselineregenerates itinand<> ALL($1::text[])fornot_inTypedDict("Name", {...})field map is skipped: its"in"key is a field name, not a filter_details_for_user_idsinkey_metadata_recovery.pynow usesfind_many_infind_manywith the samewhere; its existing tests pass unmodifiedUser Flow
Before: a contributor adds a membership filter that grows with a table and CI lets it merge
where={"user_id": {"in": user_ids}}in a proxy endpoint and open a PRtoo many bind variables in prepared statementAfter: the same PR fails CI with the fix named, and the fix handles any list size
check_unbounded_in_listswithpath:line: prisma "in" filter over user_ids has no written bound ... Chunk it with litellm.repositories.chunked_infind_many_in(table, "user_id", user_ids)(or record a real bound with# bounded-ok: <reason>) and the job passesRelevant issues
Refs #40564
Linear ticket
Refs LIT-7535
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
uv run pytest tests/test_litellm/<your_test_file>.py -v. Leave the suites (make test-unit-*,make test-unit) to CI: it finishes in ~15 minutes where a laptop takes an hour or more@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Shared setup: a local Postgres 14 on port 54917 with a
probeschema holding aLiteLLM_Configtable of 40,000 rows (id-0toid-39999).demo.pysends a rawcount(where={"param_name": {"in": ids}})over all 40,000 ids, then calls each helper with the same ids (update_many_ininsidedb.tx())Before (7688f56)
CI on a new unbounded list
python tests/code_coverage_tests/check_unbounded_in_lists.pycan't open file ... No such file or directory: nothing checks the filter, so the PR merges40,000 ids through Prisma
DATABASE_URL=postgresql://postgres@127.0.0.1:54917/litellm_test python demo.pyAfter (9c7eae6)
CI on a new unbounded list
python tests/code_coverage_tests/check_unbounded_in_lists.pyon the clean tree156 unbounded IN list(s) (148 prisma, 8 raw-sql): 156 baselined, 0 new, 0 stale baseline entries.andecho $?prints0async def scratch_violation(table, ids): return await table.find_many(where={"user_id": {"in": ids}})tolitellm/repositories/user_repository.pyand rerunecho $?prints1:40,000 ids through Prisma
DATABASE_URL=postgresql://postgres@127.0.0.1:54917/litellm_test python demo.pyTests and mutation check
pytest tests/unit/repositories/test_chunked_in.pyprints52 passed(an update writing the chunked field refused in any form, including Greptile's['old', 'new']/chunk_size=1case, a chunk equal to a hand-written filter, sizes 0, 1, 5,000, 5,001 and 12,345, customchunk_sizesetting the query count,chunk_sizebounds 1 and 30,000 accepted and 0 / -1 / 30,001 refused before any query, dedup, sums, AND composition, same-field refusal, requiredatomicity, and a query-builder check that the composed filter renders like a hand-written one)pytest tests/test_litellm/test_check_unbounded_in_lists.pyprints73 passed(new finding fails, stale entry fails, line shifts keep the key, a replaced expression on the same field fails,--update-baseline, helper exemption, constant spread, functional TypedDict field maps skipped while other calls' filters are still flagged)pytest tests/test_litellm/proxy/spend_tracking/test_key_metadata_recovery.pyprints34 passed: the 32 existing tests unmodified, plus 12,001 user ids going out as 5,000 / 5,000 / 2,001 chunks with every result merged, and a failing later chunk still logged and treated as no detailstests/integration/database/test_chunked_in_lists.pyagainst real Postgres prints5 passed: a raw 40,000-idcount/update_many/delete_manyis rejected while each helper handles the same ids, andattach_user_detailsfills every email for 40,000 users. It runs in the CircleCIdatabasegroupwheredropped from the chunk filter, or same-field check off or stopping at the top levelfind_many_inkeeping only the last pageatomicitygiven a default--update-baselinedropping unscanned entries or keeping fixed oneschunk_sizebounds loosened or tightened by one on either side, the check removed, the max raised to 32,767, orchunk_sizeignored in the range or the slicex.TypedDict, taking the first argument, or missingfields=ortyping_extensions_details_for_user_idsreverted to the raw filter, or chunks sent as tuplesType
🆕 New Feature
🚄 Infrastructure
Caveats (if any)
Medium
update_many_in/delete_many_inover 5,000 values run as several statementsatomicity="caller_transaction"records that choiceper_chunk_okleaves earlier chunks applied if a later one failsnot_incannot be chunked, so those sites need<> ALL($1::text[])or a relation filterLow
group_by_inortake/skip/order/distinct: none of them survive a split--update-baselinefrom x import NAMEconstants are not resolved across modules; no current finding needs itcheck_batch_cost.pyfalse positive was a spread of a local tuple constant, now counted as fixedfind_manyon its own; the migration is for consistency and for the cases it does not splitfind_manychunking is partial (Bug:findMany()with two largeinfilters with arrays of32767items each fails withAssertion violation,too many bind variables in prepared statementprisma/orm#21802)misc / Run testsred is main's own:test_price_map_has_no_duplicate_keysfailed on main until fix(cost-map): remove duplicate openrouter/perceptron/perceptron-mk1.5 entry #43273 (4d7aa89), which is newer than this branch's merge basecodecov/patchreadschunked_in.pyat 61% becausetests/unitruns only in the CircleCIunitjob, started by therun-cilabeltests/unit/repositories/test_chunked_in.pycovers it at 96% (52 passed); the two missed lines are thecase _arm of_logical_clausespy/mixed-returnsnotes (alerts 13011, 13012) on_as_clausesand_logical_clausesmatcharm returns,case _included, so no path falls through toNone; left as is since a push to appease a note resets the bot verdictsci/circleci: local_testing_part1red is main's own:test_get_model_info_bedrock_cross_region_capability_parityfails at this tip and passes on the tip merged with mainci/circleci: local_testing_part2red predates this PR:test_openai_stream_options_call_text_completionfails identically at the merge base 8694c3c, with'MockValSer' object is not an instance of 'SchemaSerializer'Final Attestation
Note
Medium Risk
Touches proxy spend metadata DB reads and adds broad CI rules for membership filters; chunked writes can be partially applied unless run in a transaction.
Overview
Adds CI enforcement and a repository helper so Prisma/SQL
INlists cannot silently exceed Postgres’s 32,767 bind-parameter limit.A new
litellm.repositories.chunked_inmodule exposesfind_many_in,count_in,update_many_in, anddelete_many_in, which dedupe values, run 5k-value chunks (with optional extrawhere), and merge results; writes require an explicitatomicitychoice. Related Prisma table protocols were added for typing.Code Quality now runs
check_unbounded_in_lists.py, which AST-scanslitellm/andenterprise/for unbounded Prisma"in"/"not_in"filters and runtime-spliced raw SQLIN (patterns. ~156 existing sites are grandfathered inunbounded_in_baseline.txt; new or stale baseline entries fail the job.One production migration: spend-log key metadata user lookup (
_details_for_user_ids) usesfind_many_ininstead of a singlefind_manywithuser_id: {in: ...}. Unit, integration (40k rows), and checker tests cover the helper and CI behavior.Reviewed by Cursor Bugbot for commit 9c7eae6. Bugbot is set up for automated code reviews on this repo. Configure here.