Skip to content

Conversation

@ili-ad
Copy link

@ili-ad ili-ad commented Jan 6, 2026

Summary

Fix Postgres crash in erpnext.controllers.queries.item_query caused by MySQL-style pagination (LIMIT start, page_len).

Problem

On Postgres, link-field search for Items (e.g. Sales Invoice → Item field autocomplete) can fail with:


psycopg2.errors.SyntaxError: LIMIT #,# syntax is not supported
HINT: Use separate LIMIT and OFFSET clauses.

Root cause: item_query uses LIMIT %(start)s, %(page_len)s which is valid in MariaDB/MySQL but invalid in Postgres.

Fix

Switch pagination in item_query to the portable form:

  • from: LIMIT %(start)s, %(page_len)s
  • to: LIMIT %(page_len)s OFFSET %(start)s

This syntax is supported by both Postgres and MariaDB/MySQL.

Why this change

  • Restores Item search/autocomplete functionality on Postgres
  • Keeps behavior unchanged on MariaDB/MySQL (portable SQL)
  • Minimal surface area: single-line change in erpnext/controllers/queries.py

Tests

  • pre-commit run --all-files
  • Manual: Sales Invoice → Item field search no longer errors on Postgres

Notes

No business logic changes; server-side query only.

@github-actions github-actions bot added the needs-tests This PR needs automated unit-tests. label Jan 6, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 6, 2026

📝 Walkthrough

Walkthrough

The change updates the SQL pagination in item_query within erpnext/controllers/queries.py, replacing MySQL-style LIMIT start, page_len with standard SQL LIMIT page_len OFFSET start. No other query logic was modified.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 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 'fix(postgres): use LIMIT/OFFSET in item_query' directly describes the main change: switching from MySQL-style LIMIT pagination to portable LIMIT/OFFSET syntax in the item_query function.
Description check ✅ Passed The description clearly explains the Postgres crash caused by MySQL-style pagination, the specific fix (switching to portable LIMIT/OFFSET syntax), and the rationale for the change.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

Copy link
Contributor

@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: 0

🧹 Nitpick comments (1)
erpnext/controllers/queries.py (1)

260-260: Correct fix for PostgreSQL compatibility!

The change from MySQL-specific LIMIT offset, count syntax to the standard SQL LIMIT count OFFSET offset syntax correctly addresses the PostgreSQL error while maintaining compatibility with MySQL/MariaDB.

Optional: align style with the rest of the file

For consistency with other pagination clauses in this file (lines 59, 116, 152, 295), consider using lowercase offset and removing the trailing space:

-		limit %(page_len)s OFFSET %(start)s """.format(
+		limit %(page_len)s offset %(start)s""".format(

This is purely a style suggestion and does not affect functionality.

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 57624e1 and 82f323f.

📒 Files selected for processing (1)
  • erpnext/controllers/queries.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-16T05:33:58.723Z
Learnt from: Abdeali099
Repo: frappe/erpnext PR: 51078
File: erpnext/accounts/doctype/financial_report_template/financial_report_engine.py:486-491
Timestamp: 2025-12-16T05:33:58.723Z
Learning: In ERPNext/Frappe codebase, query.run(as_dict=True) returns frappe._dict objects that support both dict-style access (obj["key"]) and attribute-style access (obj.key). Therefore, attribute access on query results is valid and will not raise AttributeError. When reviewing Python code, prefer attribute access (obj.key) for readability where the key is known to exist, but ensure existence checks or fallback handling if there is any doubt about missing keys.

Applied to files:

  • erpnext/controllers/queries.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Summary

@ruthra-kumar ruthra-kumar self-assigned this Jan 8, 2026
@ruthra-kumar ruthra-kumar added the postgres issue/pr related to postgres label Jan 16, 2026
@ruthra-kumar
Copy link
Member

@mergify rebase

@mergify
Copy link
Contributor

mergify bot commented Jan 24, 2026

rebase

✅ Branch has been successfully rebased

@ruthra-kumar ruthra-kumar force-pushed the fix/postgres-item-query-limit-offset branch from 82f323f to 2f3bf18 Compare January 24, 2026 07:53
@codecov
Copy link

codecov bot commented Jan 24, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.22%. Comparing base (f5378b6) to head (2f3bf18).
⚠️ Report is 46 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #51557   +/-   ##
========================================
  Coverage    79.22%   79.22%           
========================================
  Files         1167     1167           
  Lines       122103   122103           
========================================
+ Hits         96740    96741    +1     
+ Misses       25363    25362    -1     
Files with missing lines Coverage Δ
erpnext/controllers/queries.py 53.92% <ø> (ø)

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ruthra-kumar
Copy link
Member

@ili-ad

Traceback (most recent call last):
  File "apps/frappe/frappe/app.py", line 121, in application
    response = frappe.api.handle(request)
  File "apps/frappe/frappe/api/__init__.py", line 63, in handle
    data = endpoint(**arguments)
  File "apps/frappe/frappe/api/v1.py", line 40, in handle_rpc_call
    return frappe.handler.handle()
           ~~~~~~~~~~~~~~~~~~~~~^^
  File "apps/frappe/frappe/handler.py", line 53, in handle
    data = execute_cmd(cmd)
  File "apps/frappe/frappe/handler.py", line 86, in execute_cmd
    return frappe.call(method, **frappe.form_dict)
           ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "apps/frappe/frappe/__init__.py", line 1124, in call
    return fn(*args, **newargs)
  File "apps/frappe/frappe/utils/typing_validations.py", line 36, in wrapper
    return func(*args, **kwargs)
  File "apps/frappe/frappe/utils/caching.py", line 248, in inner
    ret = func(*args, **kwargs)
  File "apps/frappe/frappe/desk/search.py", line 51, in search_link
    results = search_widget(
    	doctype,
    ...<7 lines>...
    	link_fieldname=link_fieldname,
    )
  File "apps/frappe/frappe/utils/typing_validations.py", line 36, in wrapper
    return func(*args, **kwargs)
  File "apps/frappe/frappe/desk/search.py", line 140, in search_widget
    return frappe.call(
           ~~~~~~~~~~~^
    	query,
     ^^^^^^
    ...<9 lines>...
    	link_fieldname=link_fieldname,
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
  File "apps/frappe/frappe/__init__.py", line 1124, in call
    return fn(*args, **newargs)
  File "apps/frappe/frappe/utils/typing_validations.py", line 36, in wrapper
    return func(*args, **kwargs)
  File "apps/frappe/frappe/__init__.py", line 1551, in wrapper
    return fn(**kwargs)
  File "apps/erpnext/erpnext/controllers/queries.py", line 244, in item_query
    return frappe.db.sql(
           ~~~~~~~~~~~~~^
    	"""select
     ^^^^^^^^^
    ...<28 lines>...
    	as_dict=as_dict,
     ^^^^^^^^^^^^^^^^
    )
    ^
  File "apps/frappe/frappe/database/postgres/database.py", line 234, in sql
    return super().sql(modify_query(query), modify_values(values), *args, **kwargs)
           ~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "apps/frappe/frappe/database/database.py", line 272, in sql
    self.execute_query(query, values)
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "apps/frappe/frappe/database/database.py", line 372, in execute_query
    return self._cursor.execute(query, values)
           ~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
psycopg2.errors.UndefinedTable: missing FROM-clause entry for table "tabitem"
LINE 2:    tabItem.name , item_name, item_group, customer_code, if(l...
           ^

request.js:476:15

Query requires more changes

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

Labels

needs-tests This PR needs automated unit-tests. postgres issue/pr related to postgres

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants