Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 28 additions & 4 deletions .github/copilot-instructions.md
Original file line number Diff line number Diff line change
Expand Up @@ -58,17 +58,38 @@ Testing
- Must write all failing tests first (red), then implement until all pass (green).
- Must cover all distinct combinations; each test must state its scenario in the ScalaDoc.
- Must update `SPEC.md` after all tests pass with the confirmed test case table.
- When a unit test adds value — write one:
- Method has any own logic: branching (`if`/`match`), exception handling or swallowing, non-trivial transformation, config/resource read, or reflection.
- When to add to `jmf-rules.txt` instead of writing a unit test:
- Body is a single call with no own logic: forwards to another overload, calls its non-deprecated replacement, returns a field, or wraps a constructor with no transformation.
- Litmus: "Does this method have any logic of its own?" — No → JMF.
- Global rule collision check (CRITICAL):
- When adding any new method, check if it matches a pattern in the `# GLOBAL RULES` section of `jmf-rules.txt` (line ~22+).
- If method name matches a global rule AND the method has domain logic: immediately create an INCLUDE rescue rule (`+FQCN#method(*)`) in the `# INCLUDE RULES` section of `jmf-rules.txt`.
- High-risk method names: refer to `jmf-rules.txt` GLOBAL RULES section for complete list; most common collisions: `apply()`, `toString()`, `equals()`, `copy()`, `name()`, `groups()`, `optionalAttributes()`.
- Rationale: Broad global rules designed for compiler-generated boilerplate can silently hide coverage for domain methods. INCLUDE rules rescue specific methods from broad exclusions.
- Example: If adding `def apply(id: String): Record`, add `+*Record$#apply(*) id:keep-record-factory` to rescue from the `*$*#apply(*)` global rule in GLOBAL RULES section.
- Review rule — JMF drift check: when modifying a method that appears in `jmf-rules.txt`, verify its body still qualifies; if own logic has been added, remove the JMF rule and write a unit test instead.

Tooling
- Must format with scalafmt (`.scalafmt.conf`); lint with scalastyle (`scalastyle-config.xml`) or wartremover as configured.
- Compiler warnings treated as errors where configured; coverage ≥ 80% via sbt-jacoco (excluding JMF-filtered methods in `jmf-rules.txt`).

Coverage filtering (JMF)
- A method qualifies for a JMF filter rule if it meets at least one criterion:
- No added value: trivial delegate, one-liner factory, or pure field accessor — a test adds no assurance beyond testing the delegated method.
- Not coverable without integration tests: requires a DB/external system; same path already exercised by integration tests.
- Qualifying patterns: deprecated single-call delegates; `columnLabel: String` overloads that only call `columnNumber(label)` + Int-based overload; one-liner factory wrappers; implicit single-field accessors.
- A method qualifies when its body is a single call with no own logic (see Testing section for the full decision rule and qualifying patterns).
- Must not add JMF rules for methods with branching logic, error handling, or non-trivial transformations.
- Rules file: `jmf-rules.txt`; one rule per line, `#` comments and blank lines ignored.
- Rule syntax: `<FQCN_glob>#<method_glob>(<descriptor_glob>) [FLAGS] [PREDICATES]`
- FQCN_glob: dot-form class pattern (`*`, `*.model.*`, `com.example.*`; `$` for inner/companion classes).
- method_glob: glob on method name (`copy`, `get*`, `$anonfun$*`, `*_$eq`).
- descriptor_glob: JVM descriptor `(args)ret`; omitting or `(*)` means any args/any return. Must use JVM internal format: `(I)*` not `(int)`, `(Z)*` not `(boolean)`, `(Ljava/lang/String;)*` not `(java.lang.String)`. Non-matching descriptors are silently ignored — no warning is emitted.
- FLAGS (space/comma separated): `public`, `protected`, `private`, `synthetic`, `bridge`, `static`, `abstract`.
- PREDICATES: `ret:<glob>` (return type), `id:<string>` (log label), `name-contains:<s>`, `name-starts:<s>`, `name-ends:<s>`.
- Every rule Must include an `id:` label for traceability.
- Adoption order: start with CONSERVATIVE (case-class boilerplate, compiler synthetics), then STANDARD; use AGGRESSIVE only for DTO/auto-generated packages.
- Prefer narrow package scopes; prefer `synthetic`/`bridge` flags for compiler artifacts over broad wildcards.
- Must prefix project-specific FQCN globs with `*` so they match the full qualified class name (e.g. `*QueryResult#noMore()`, not `QueryResult#noMore()`).
- When adding a project-specific rule, add it under the `# PROJECT RULES` section with a comment explaining why the method qualifies.

Quality gates
- sbt "testOnly *UnitTests" # unit tests, no DB needed
Expand All @@ -83,10 +104,13 @@ Common pitfalls to avoid
- Logging: never pre-build log strings; always pass args lazily (call-by-name).
- Cleanup: remove unused imports/variables (scalac `-Ywarn-unused`).
- Stability: avoid changing externally-visible method signatures, SQL output, or parameter binding order.
- JMF silent failures: rules with non-matching FQCN or descriptor globs are silently ignored; always verify new rules take effect by comparing JaCoCo method-level output before and after.
- Scala reflection: case classes used with `currentMirror.reflectClass` in tests must be top-level (package scope), not inner classes — Scala reflection cannot handle inner class mirrors.

Learned rules
- Must not change `QueryResultRow` getter return types or `stringPerConvention` output for existing scenarios.
- Must not change externally-visible SQL generation patterns without updating dependent tests.
- Scala 2.12 compiler artifacts that generate coverable bytecode: `$anonfun$` lambdas (ACC_SYNTHETIC), `$deserializeLambda$` (private static, not synthetic), Function1 mixin forwarders (`andThen`/`compose`), value class `$extension` methods, `$default$` parameter methods, Iterator trait mixin (~80+ forwarders). These are JMF candidates, not unit-test candidates.

Repo additions
- Project name: balta
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,12 @@ class QueryResultRow private[classes](val rowNumber: Int,

/**
* Extracts a value from the row by column number.
* @param column - the number of the column, 1 based
* @param column - the 1-based column number (JDBC convention); note: prior to this fix the parameter was
* incorrectly treated as 0-based (direct vector index), causing column 1 to silently return
* the value of the second column
* @return - the value stored in the column, type `Any` is for warningless comparison with any type
*/
def apply(column: Int): Option[Any] = getObject(column - 1)
def apply(column: Int): Option[Any] = getObject(column)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why this change?
It might be breaking, AFAIK, JDBC has 0-based indexing of columns, which is counterintuitive.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Pls confirm if it is or is not a bug, I have found it during review using copilot. I have read a lot about it, and the bug looks legit. I am not deep familiar with this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You are right, it's 1 based. So it's a bug.
Would mention it in the release notes, and mark it as breaking change.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think that previously there was a bug in the codebase, but please document it. On the parameter level ideally, instead of this:

* @param column - the number of the column, 1 based

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Doc improved in commit - 1330c03.

/**
* Extracts a value from the row by column name.
* @param columnLabel - the name of the column
Expand Down
7 changes: 7 additions & 0 deletions balta/src/test/resources/database-persist.properties
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
# jdbc settings — test fixture with persist=true
test.jdbc.url=jdbc:postgresql://localhost:5432/mag_db

test.jdbc.username=mag_owner
test.jdbc.password=changeme

test.persist.db=true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Really persist to true?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This file is used only in unit test fixture - explicity name is there.
Do you see some serious problem with its existence?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, I still don't get why is it needed?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yeah the only usecase for this setting to true in the past, for me, was if I wanted to query and see what's in the DB after the tests finished

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

yes, It is here from the begin to allow to keep state after test for manual review.

Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,97 @@
package za.co.absa.db.balta

import org.scalatest.funsuite.AnyFunSuiteLike
import za.co.absa.db.balta.classes.DBFunction.DBFunctionWithPositionedParamsOnly
import za.co.absa.db.balta.classes.DBTable
import za.co.absa.db.balta.classes.inner.ConnectionInfo
import za.co.absa.db.balta.classes.inner.Params.{NamedParams, OrderedParams}
import za.co.absa.db.balta.typeclasses.QueryParamType

class DBTestSuiteUnitTests extends AnyFunSuiteLike {

test("connectionInfoFromResourceConfig reads local resource file correctly") {
val connectionInfo = DBTestSuite.connectionInfoFromResourceConfig("/database.properties")
assert(connectionInfo.dbUrl == "jdbc:postgresql://localhost:5432/mag_db")
assert(connectionInfo.username == "mag_owner")
assert(connectionInfo.password == "changeme")
assert(!connectionInfo.persistData)
}

test("connectionInfoFromResourceConfig reads persistData true when configured") {
val connectionInfo = DBTestSuite.connectionInfoFromResourceConfig("/database-persist.properties")
assert(connectionInfo.persistData)
}

test("connectionInfo with persistDataOverride=Some(true) overrides config value") {
val suite = new ConcreteSuite(Some(true))
val info = suite.exposedConnectionInfo
assert(info.persistData)
}

test("connectionInfo with persistDataOverride=Some(false) overrides config value") {
val suite = new ConcreteSuite(Some(false))
val info = suite.exposedConnectionInfo
assert(!info.persistData)
}

test("connectionInfo with persistDataOverride=None uses config value") {
val suite = new ConcreteSuite(None)
val info = suite.exposedConnectionInfo
assert(!info.persistData)
}

test("table helper creates DBTable with provided name") {
val suite = new ConcreteSuite(None)
val table = suite.exposedTable("my_table")
assert(table == DBTable("my_table"))
}

test("function helper creates DBFunction with provided name") {
val suite = new ConcreteSuite(None)
val fn = suite.exposedFunction("my_fn")
assert(fn.isInstanceOf[DBFunctionWithPositionedParamsOnly])
assert(fn.functionName == "my_fn")
}

test("named add helper delegates to Params.add") {
val suite = new ConcreteSuite(None)
val params: NamedParams = suite.exposedAdd("id", 7)
assert(params.size == 1)
assert(params("id").sqlEntry == "?")
}

test("named addNull helper delegates to Params.addNull") {
val suite = new ConcreteSuite(None)
val params: NamedParams = suite.exposedAddNull("missing")
assert(params.size == 1)
assert(params("missing").sqlEntry == "NULL")
assert(params("missing").equalityOperator == "IS")
}

test("ordered add helper delegates to Params.add") {
val suite = new ConcreteSuite(None)
val params: OrderedParams = suite.exposedAdd(1)
assert(params.size == 1)
assert(params.values.head.sqlEntry == "?")
}

test("ordered addNull helper delegates to Params.addNull") {
val suite = new ConcreteSuite(None)
val params: OrderedParams = suite.exposedAddNull[QueryParamType.NULL.type]()
assert(params.size == 1)
assert(params.values.head.sqlEntry == "NULL")
assert(params.values.head.equalityOperator == "IS")
}

private class ConcreteSuite(override val persistDataOverride: Option[Boolean])
extends DBTestSuite(persistDataOverride) {
def exposedConnectionInfo: ConnectionInfo = connectionInfo
def exposedTable(tableName: String): DBTable = table(tableName)
def exposedFunction(functionName: String): DBFunctionWithPositionedParamsOnly = function(functionName)
def exposedAdd[T: QueryParamType](name: String, value: T): NamedParams = add(name, value)
def exposedAddNull(name: String): NamedParams = addNull(name)
def exposedAdd[T: QueryParamType](value: T): OrderedParams = add(value)
def exposedAddNull[T: QueryParamType](): OrderedParams = addNull[T]()
}
}

Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/*
* Copyright 2023 ABSA Group Limited
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package za.co.absa.db.balta.classes

import org.scalatest.funsuite.AnyFunSuiteLike
import za.co.absa.db.balta.classes.DBFunction.{DBFunctionWithNamedParamsToo, DBFunctionWithPositionedParamsOnly}

class DBFunctionUnitTests extends AnyFunSuiteLike {

test("DBFunction.apply creates a positioned-params instance with no params") {
val fn = DBFunction("my_schema.my_function")
assert(fn.isInstanceOf[DBFunctionWithPositionedParamsOnly])
assert(fn.params.isEmpty)
assert(fn.functionName == "my_schema.my_function")
}

test("setParam by position auto-increments the key") {
val fn = DBFunction("fn")
.setParam(10)
.setParam("hello")
.setParam(true)
assert(fn.params.size == 3)
assert(fn.params.keys.toList == List(Left(1), Left(2), Left(3)))
}

test("setParam by position returns DBFunctionWithPositionedParamsOnly") {
val fn = DBFunction("fn").setParam(42)
assert(fn.isInstanceOf[DBFunctionWithPositionedParamsOnly])
}

test("setParam by name creates a named-params instance") {
val fn = DBFunction("fn").setParam("p_name", "value")
assert(fn.isInstanceOf[DBFunctionWithNamedParamsToo])
assert(fn.params.size == 1)
assert(fn.params.keys.head == Right("p_name"))
}

test("mixing positioned and named params preserves order") {
val fn = DBFunction("fn")
.setParam(1)
.setParam(2)
.setParam("named", "val")
assert(fn.params.size == 3)
assert(fn.params.keys.toList == List(Left(1), Left(2), Right("named")))
}

test("clear resets params to empty") {
val fn = DBFunction("fn").setParam(1).setParam(2).clear()
assert(fn.params.isEmpty)
assert(fn.functionName == "fn")
}

test("clear returns DBFunctionWithPositionedParamsOnly") {
val fn = DBFunction("fn").setParam("p", "v").clear()
assert(fn.isInstanceOf[DBFunctionWithPositionedParamsOnly])
}

test("setParamNull by name adds NULL param with named key") {
val fn = DBFunction("fn").setParamNull("p_null")
assert(fn.params.size == 1)
assert(fn.params.keys.head == Right("p_null"))
assert(fn.params.values.head.sqlEntry == "NULL")
assert(fn.params.values.head.equalityOperator == "IS")
}

test("setParamNull by position appends NULL as next index") {
val fn = DBFunction("fn").setParam(10).setParamNull()
assert(fn.params.size == 2)
assert(fn.params.keys.toList == List(Left(1), Left(2)))
assert(fn.params.values.toList.last.sqlEntry == "NULL")
assert(fn.params.values.toList.last.equalityOperator == "IS")
}
}
Loading
Loading