Skip to content

Add defensive copies for mutable DTO and AST collections #313

Description

@kaklakariada

Problem

Several request DTOs, metadata objects, and SQL AST nodes store caller-provided mutable collections directly. Some getters also return those mutable collections directly. This makes objects that look like value objects vulnerable to silent state corruption after construction.

A caller can mutate the original list/map after passing it to a constructor, or in some cases mutate the collection returned by a getter. That can change request metadata or AST structure several frames away from the code that originally built it.

Confirmed locations

The following classes currently store caller-owned collections without defensive copies:

  • src/main/java/com/exasol/adapter/request/RefreshRequest.java: selected tables list. getTables() returns the stored list directly.
  • src/main/java/com/exasol/adapter/request/SetPropertiesRequest.java: properties map. getProperties() returns the stored map directly.
  • src/main/java/com/exasol/adapter/request/PushDownRequest.java: involvedTablesMetadata and selectListDataTypes lists. Both getters return stored lists directly.
  • src/main/java/com/exasol/adapter/metadata/TableMetadata.java: columns list. getColumns() returns the stored list directly.
  • src/main/java/com/exasol/adapter/metadata/SchemaMetadata.java: tables list. getTables() returns the stored list directly.
  • src/main/java/com/exasol/adapter/metadata/SchemaMetadataInfo.java: properties map. getProperties() returns the stored map directly.
  • src/main/java/com/exasol/adapter/sql/SqlOrderBy.java: expressions, isAsc, and nullsLast lists. Getters return unmodifiable views, but the constructor still stores the original lists, so later mutation of the constructor arguments changes internal state.
  • src/main/java/com/exasol/adapter/sql/SqlFunctionScalarJsonValue.java: arguments list. getArguments() returns the stored list directly.
  • src/main/java/com/exasol/adapter/sql/SqlPredicateInConstList.java: inArguments list. Getter returns an unmodifiable view, but the constructor stores the original list.

SqlOrderBy invariant is checked too late

SqlOrderBy stores three parallel lists: expressions, sort directions, and null ordering. PushdownSqlRenderer.visit(SqlOrderBy) checks that the three lists have equal sizes and throws F-VSCOMJAVA-33 only at render time.

Because SqlOrderBy does not validate this in the constructor and does not defensively copy the lists, invalid state can be created directly or introduced later by mutating a constructor argument. The diagnostic then appears during rendering instead of at the point where the invalid order-by node was built.

Expected behavior

These classes should behave like stable DTO/value/AST objects after construction:

  • Defensively copy mutable collection inputs in constructors.
  • Return unmodifiable collections from getters.
  • Treat null collection inputs as empty collections where the current API is tolerant of null.
  • For maps that may contain null values, use new HashMap<>(...) rather than Map.copyOf(...), because Map.copyOf(...) rejects null keys/values.
  • For lists, use List.copyOf(...) when null elements are not supported, or new ArrayList<>(...) plus an unmodifiable getter when null elements must remain supported.
  • Validate the SqlOrderBy parallel-list size invariant in the constructor while keeping the renderer check as defence in depth.

Suggested tests

Add focused tests that mutate constructor inputs and verify the constructed object does not change. Also verify getters are unmodifiable where applicable, and add constructor-time tests for mismatched SqlOrderBy list sizes.

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