Skip to content

OF-3277: Introduce helper method to determine database 'limit' keyword#3330

Open
guusdk wants to merge 1 commit into
igniterealtime:mainfrom
guusdk:OF-3277_DB-limit-keyword-helper
Open

OF-3277: Introduce helper method to determine database 'limit' keyword#3330
guusdk wants to merge 1 commit into
igniterealtime:mainfrom
guusdk:OF-3277_DB-limit-keyword-helper

Conversation

@guusdk
Copy link
Copy Markdown
Member

@guusdk guusdk commented May 14, 2026

The SQL LIMIT clause is not portable across the databases Openfire supports. SQL Server, for example, uses SELECT TOP.

Up until now, code had to interrogate the type of database to find out what variant to be used. In this commit, this information is exposed as metadata.

Small changes have been applid to the pubsub persistence implementation. It is expected that the Monitoring plugin (and others) benefit from this, too.

The SQL LIMIT clause is not portable across the databases Openfire supports. SQL Server, for example, uses `SELECT TOP`.

Up until now, code had to interrogate the type of database to find out what variant to be used. In this commit, this information is exposed as metadata.

Small changes have been applid to the pubsub persistence implementation. It is expected that the Monitoring plugin (and others) benefit from this, too.
@guusdk guusdk requested a review from Copilot May 14, 2026 10:08
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 14, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a5bcc108-30d5-4467-8eb2-d8c30b9cd4dd

📥 Commits

Reviewing files that changed from the base of the PR and between 3e6245d and bc0caf5.

📒 Files selected for processing (3)
  • xmppserver/src/main/java/org/jivesoftware/database/DbConnectionManager.java
  • xmppserver/src/main/java/org/jivesoftware/openfire/pubsub/DefaultPubSubPersistenceProvider.java
  • xmppserver/src/test/java/org/jivesoftware/database/DbConnectionManagerTest.java

📝 Walkthrough

Walkthrough

This PR adds a public API to DbConnectionManager for database-specific result-set limit syntax and applies it to eliminate hardcoded database checks. A new ResultSetLimitKeyword enum maps each database type (SQL Server, Oracle, DB2, ANSI) to its keyword (TOP, FETCH_FIRST, or LIMIT) and metadata indicating whether the keyword is prefix- or suffix-style. DefaultPubSubPersistenceProvider.getPublishedItems() is refactored to use this API when selecting between prepared statements and binding the max parameter. New JUnit tests validate the keyword mappings and prefix behavior across database types.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description accurately describes the changeset and its motivation: exposing database-specific SQL LIMIT keyword metadata to avoid hard-coded database type checks.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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


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

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR (OF-3277) centralizes database-specific SQL row-limiting behavior by exposing metadata from DbConnectionManager, reducing the need for scattered database-type checks (eg. LIMIT vs SQL Server TOP).

Changes:

  • Added ResultSetLimitKeyword metadata (keyword + prefix/suffix positioning) to DbConnectionManager / DatabaseType.
  • Updated PubSub persistence logic to select the appropriate query variant based on the new metadata.
  • Added unit tests that validate the mapping for SQL Server, Oracle, DB2, and ANSI-style databases.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
xmppserver/src/main/java/org/jivesoftware/database/DbConnectionManager.java Introduces ResultSetLimitKeyword and helper methods to expose per-database row-limiting keyword metadata.
xmppserver/src/main/java/org/jivesoftware/openfire/pubsub/DefaultPubSubPersistenceProvider.java Replaces database-type branching with the new row-limiting keyword metadata when loading last published items.
xmppserver/src/test/java/org/jivesoftware/database/DbConnectionManagerTest.java Adds test coverage for the new keyword/prefix metadata across key database types.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@guusdk guusdk added this to the 5.1.0 milestone May 14, 2026
@stokito
Copy link
Copy Markdown
Member

stokito commented May 14, 2026

Looks like start of re-inventing Hibernate Dialect.
Maybe we can't use it because of it's bad performance but maybe something can be helpful there
https://github.com/hibernate/hibernate-orm/blob/main/hibernate-core/src/main/java/org/hibernate/dialect/pagination/SQLServer2012LimitHandler.java

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants