Skip to content

Make SqlLimit immutable and keep constructor validation invariant #308

Description

@kaklakariada

Problem

SqlLimit validates constructor arguments but exposes public setters that bypass the same invariant. The constructor rejects negative limit and offset values:

if (offset < 0 || limit < 0) {
    throw new IllegalArgumentException(...);
}

However, setLimit(int) and setOffset(int) assign values directly, so callers can create a SqlLimit instance in a state that the constructor would reject. PushdownSqlRenderer later renders these values directly via getLimit() and getOffset().

Evidence

  • src/main/java/com/exasol/adapter/sql/SqlLimit.java: fields limit and offset are mutable and the public setters do not validate negative values.
  • src/main/java/com/exasol/adapter/request/parser/PushdownSqlParser.java: parseLimit() constructs limits through new SqlLimit(numElements, offset), which applies validation.
  • src/main/java/com/exasol/adapter/request/renderer/PushdownSqlRenderer.java: visit(SqlLimit) serializes the current values directly.
  • rg only finds setLimit() and setOffset() usage in SqlLimitTest, so production code does not appear to rely on mutation.

Expected behavior

SqlLimit should preserve the same non-negative invariant for its lifetime. Since it represents an AST node that is constructed and then traversed by parsers/renderers/visitors, it should be immutable like the surrounding select structure expects.

Suggested fix

  • Make limit and offset final.
  • Remove setLimit(int) and setOffset(int).
  • Remove or replace the setter-specific tests.
  • Fix the validation message from "greater than zero" to "greater than or equal to zero", because 0 is currently accepted by the constructor and is valid behavior.

Metadata

Metadata

Assignees

Labels

bugUnwanted / harmful behavior

Type

Fields

No fields configured for Bug.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions