#52: Use type classe for AllowedParamTypes#58
Conversation
* `AllowedParamTypes` renamed to `QueryParamType` and changed to typeclass * `SetterFnc` renamed and refactored into `QueryParamValue` * Moved Postgres specific stuff into its own package * Dropped deprecated `JsonBString` class and support * Deprecated `addNull` functions instead implemented a constant `QueryParamValue.NULL` * Dependency on Postgres made optional * Added scalas options to project * Incremented sbt version * Enhanced `.gitignore` to better cover VS Code files
benedeki
requested review from
lsulak,
miroslavpojer and
salamonpavel
as code owners
benedeki
added
the
breaking change
JaCoCo 'balta' module code coverage report - scala 2.12.18
|
WalkthroughReplaces the SetterFnc/AllowedParamTypes system with QueryParamType/QueryParamValue typeclasses, moves several classes into an Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (DBTable/DBFunction/Params)
participant QType as QueryParamType[T]
participant QValue as QueryParamValue
participant Prep as PreparedStatement
Caller->>QType: implicitly[QueryParamType[T]].toQueryParamValue(value)
Note right of QType `#eef3ff`: produces QueryParamValue (Object/Simple/Null)
QType-->>QValue: QueryParamValue
Caller->>Caller: collect List[QueryParamValue]
Caller->>Prep: prepareStatement(sql)
loop bind params
Caller->>QValue: queryValue.assign.foreach(f => f(prep, idx))
alt assign present
Note right of Prep `#e6fff2`: binds parameter at idx
else no assign (NULL)
Note right of Prep `#fff7e6`: sqlEntry = "NULL", no bind
end
end
Caller->>Prep: executeQuery / executeUpdate
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/ConnectionInfo.scala (1)
19-27: Incorrect doc comment forConnectionInfo.The doc comment describes "a function that sets a parameter of a prepared statement," which does not match the
ConnectionInfocase class below. This appears to be a stale comment from a previous or different class.-/** - * This is a function that sets a parameter of a prepared statement. - * - * @param dbUrl - the JDBC URL of the database - * @param username - the username to use when connecting to the database - * @param password - the password to use when connecting to the database - * @param persistData - whether to persist the data to the database (usually false for tests, set to true for - * debugging purposes) - */ +/** + * Connection information for establishing a database connection. + * + * @param dbUrl - the JDBC URL of the database + * @param username - the username to use when connecting to the database + * @param password - the password to use when connecting to the database + * @param persistData - whether to persist the data to the database (usually false for tests, set to true for + * debugging purposes) + */
🧹 Nitpick comments (11)
balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala (1)
54-58: Consider updating deprecatedaddNullusage.The
addNullmethod is deprecated per this PR (in favor ofadd(paramName, QueryParamType.NULL)). While keeping deprecated methods in tests for backward compatibility validation is reasonable, consider updating or adding parallel tests with the new pattern.balta/src/main/scala/za/co/absa/db/balta/classes/DBConnection.scala (1)
33-35: Unconventional formatting and unused parameter.The closing parenthesis placement is unusual. Additionally,
persistDatais passed toConnectionInfobut theConnectionInfoobject is only used to extract connection parameters -persistDataisn't used within thisapplymethod itself. This is consistent with the otherapplyoverload, but worth noting that the parameter is only stored for later use by callers who access theConnectionInfo.Consider reformatting for consistency:
- def apply(dbUrl: String, username: String, password: String, persistData: Boolean = false): DBConnection = - apply(ConnectionInfo(dbUrl, username, password, persistData) -) + def apply(dbUrl: String, username: String, password: String, persistData: Boolean = false): DBConnection = + apply(ConnectionInfo(dbUrl, username, password, persistData))balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (1)
32-90: Consider addingOption[T]support for nullable parameters.Currently, to pass a nullable value, users must explicitly check for
Noneand useQueryParamType.NULL. Adding an implicit instance forOption[T]could improve ergonomics:implicit def SQLParamOption[T: QueryParamType]: QueryParamType[Option[T]] = new QueryParamType[Option[T]] { override def toQueryParamValue(value: Option[T]): QueryParamValue = value match { case Some(v) => implicitly[QueryParamType[T]].toQueryParamValue(v) case None => QueryParamValue.NullParamValue } }This would allow
add("param", someOption)directly instead of pattern matching on the caller side.balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (1)
133-136: Deprecation annotations are appropriate.The deprecation of
addNullmethods in favor ofadd(NULL)(usingQueryParamType.NULL) aligns with the PR objectives.However, the type parameter
T: QueryParamTypeon line 156 is now unnecessary since the method body callsParams.addNull()which internally usesQueryParamType.NULL. Consider simplifying:@deprecated("Use add(NULL)", "balta 0.3.0") - protected def addNull[T: QueryParamType](): OrderedParams = { + protected def addNull(): OrderedParams = { Params.addNull() }Also applies to: 155-158
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (2)
119-122: Consider clarifying the deprecation message.The deprecation message "Use setParam(NULL)" could be more explicit. Users need to pass
QueryParamType.NULL, not justNULL. Consider updating the message to:@deprecated("Use setParam(paramName, QueryParamType.NULL)", "balta 0.3.0")This makes the migration path clearer for users.
Apply this diff to improve clarity:
- @deprecated("Use setParam(NULL)", "balta 0.3.0") + @deprecated("Use setParam(paramName, QueryParamType.NULL)", "balta 0.3.0") def setParamNull(paramName: String): DBFunctionWithNamedParamsToo = {
178-181: Consider clarifying the deprecation message.Similar to the named parameter version, the deprecation message could be more explicit about using
QueryParamType.NULL:Apply this diff:
- @deprecated("Use setParam(NULL)", "balta 0.3.0") + @deprecated("Use setParam(QueryParamType.NULL)", "balta 0.3.0") def setParamNull(): DBFunctionWithPositionedParamsOnly = {balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (3)
84-86: Unused type parameter in addNull.The type parameter
[T: QueryParamType]is declared but never used in the method body. This appears unnecessary since the method always addsQueryParamType.NULL.Consider removing the unused type parameter:
- def addNull[T: QueryParamType](): OrderedParams = { + def addNull(): OrderedParams = { new OrderedParams().add(QueryParamType.NULL) }Note: This same pattern appears in the deprecated
addNullmethod at line 151 and inDBTestSuite.scala(lines 154-157). If there's a specific reason for the type parameter (e.g., for compatibility or implicit resolution), please document it.
114-117: Consider clarifying the deprecation message.Similar to
DBFunction.scala, the deprecation message could be more explicit:Apply this diff:
- @deprecated("Use add(NULL)", "balta 0.3.0") + @deprecated("Use add(paramName, QueryParamType.NULL)", "balta 0.3.0") def addNull(paramName: String): NamedParams = {
150-153: Unused type parameter and deprecation message.This method has two issues:
- The type parameter
[T: QueryParamType]is unused (same issue as line 84).- The deprecation message could be clearer.
Apply this diff:
- @deprecated("Use add(NULL)", "balta 0.3.0") - def addNull[T: QueryParamType](): OrderedParams = { + @deprecated("Use add(QueryParamType.NULL)", "balta 0.3.0") + def addNull(): OrderedParams = { add(QueryParamType.NULL) }project/Dependencies.scala (1)
27-32: PostgreSQL now optional – good for consumers, but document the breaking aspect.Switching
postgresqlto% Optionalis a nice win for downstreams that don’t use Postgres, since they won’t get the driver transitively anymore. It is, however, a behaviour change: any existing consumers that relied on transitive inclusion of the JDBC driver will now need to addorg.postgresql:postgresqlthemselves.I’d recommend:
- Calling this out explicitly in release notes / migration guide as a breaking change.
- Ensuring tests cover both:
- Code paths that work without the driver present.
- Code paths that require Postgres, with the driver added explicitly in test dependencies.
build.sbt (1)
38-48: Enabling-deprecationand-featureis a good move; consider extending later.Adding these options at the project level is helpful for catching misuse during this refactor, and since you’re not treating warnings as errors (here), it shouldn’t break builds—just increase signal. Over time, you might also consider
-uncheckedor a curated-Xlintset, but that can wait until the dust settles.Please just verify that CI output remains manageable and that there are no unexpected new warnings across all supported Scala versions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.gitignore(1 hunks)balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala(5 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBConnection.scala(2 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala(6 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala(10 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/inner/ConnectionInfo.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala(9 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/setter/AllowedParamTypes.scala(0 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/setter/CustomDBType.scala(0 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/setter/SetterFnc.scala(0 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/simple/JsonBString.scala(0 hunks)balta/src/main/scala/za/co/absa/db/balta/implicits/package.scala(0 hunks)balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala(2 hunks)balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala(1 hunks)build.sbt(1 hunks)project/Dependencies.scala(1 hunks)project/build.properties(1 hunks)
💤 Files with no reviewable changes (5)
- balta/src/main/scala/za/co/absa/db/balta/classes/setter/CustomDBType.scala
- balta/src/main/scala/za/co/absa/db/balta/classes/setter/AllowedParamTypes.scala
- balta/src/main/scala/za/co/absa/db/balta/classes/simple/JsonBString.scala
- balta/src/main/scala/za/co/absa/db/balta/classes/setter/SetterFnc.scala
- balta/src/main/scala/za/co/absa/db/balta/implicits/package.scala
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-11T08:21:36.735Z
Learnt from: benedeki
Repo: AbsaOSS/balta PR: 55
File: project/build.properties:1-1
Timestamp: 2025-09-11T08:21:36.735Z
Learning: sbt 1.11.5 and other sbt 1.x versions support and can run on JDK 8, despite common misconceptions that newer sbt versions require JDK 11+. The AbsaOSS organization has successfully released Java 8 libraries using sbt 1.11.5, as demonstrated with the spark-data-standardization library.
Applied to files:
project/build.properties
🧬 Code graph analysis (13)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (2)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
sql(37-46)balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala (1)
assignFunc(14-16)
balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (2)
Params(28-38)Params(39-157)
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)
balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala (1)
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (2)
SimpleJsonString(29-29)SimpleJsonString(31-37)
balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala (1)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (2)
QueryParamValue(28-48)assignFunc(32-34)
balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/ConnectionInfo.scala (1)
ConnectionInfo(28-33)
balta/src/main/scala/za/co/absa/db/balta/classes/DBConnection.scala (1)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/ConnectionInfo.scala (1)
ConnectionInfo(28-33)
balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala (2)
balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala (2)
Postgres(23-35)PostgresRow(24-34)balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (2)
SimpleJsonString(29-29)SimpleJsonString(31-37)
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (1)
QueryParamType(32-92)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala (2)
PostgresQueryParamValue(8-21)PostgresObjectQueryParamValue(9-18)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (4)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
sql(37-46)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (4)
QueryParamValue(28-48)SimpleQueryParamValue(38-40)ObjectQueryParamValue(31-36)NullParamValue(42-46)balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (1)
toQueryParamValue(35-35)balta/src/main/scala/za/co/absa/db/balta/implicits/OptionImplicits.scala (1)
value(37-37)
balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (3)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (12)
Params(28-38)Params(39-157)NamedParams(93-122)OrderedParams(129-156)add(50-52)add(73-75)add(102-105)add(137-141)addNull(61-63)addNull(84-86)addNull(114-117)addNull(150-153)balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (2)
DBFunction(34-132)DBFunction(135-195)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (1)
QueryParamType(32-92)
balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (16)
QueryParamType(32-92)NULL(86-86)toQueryParamValue(35-35)toQueryParamValue(39-39)toQueryParamValue(43-43)toQueryParamValue(47-47)toQueryParamValue(51-51)toQueryParamValue(55-55)toQueryParamValue(59-59)toQueryParamValue(63-63)toQueryParamValue(67-67)toQueryParamValue(71-71)toQueryParamValue(75-75)toQueryParamValue(79-79)toQueryParamValue(83-83)toQueryParamValue(89-89)balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (4)
add(123-125)add(145-147)addNull(133-136)addNull(155-158)
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (2)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (16)
QueryParamType(32-92)toQueryParamValue(35-35)toQueryParamValue(39-39)toQueryParamValue(43-43)toQueryParamValue(47-47)toQueryParamValue(51-51)toQueryParamValue(55-55)toQueryParamValue(59-59)toQueryParamValue(63-63)toQueryParamValue(67-67)toQueryParamValue(71-71)toQueryParamValue(75-75)toQueryParamValue(79-79)toQueryParamValue(83-83)toQueryParamValue(89-89)NULL(86-86)
⏰ 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: Scala 2.11.12
🔇 Additional comments (37)
.gitignore (1)
38-40: LGTM!The additions of
.metals/,.bloop/, andmetals.sbtto the VS Code tooling section are appropriate and follow existing conventions. These entries correctly exclude Scala language server and build tool artifacts that should not be committed.balta/src/test/scala/za/co/absa/db/balta/testing/classes/DBTestingConnection.scala (1)
20-20: LGTM!Import path correctly updated to reflect the package move of
ConnectionInfoto theinnerpackage.balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala (1)
20-20: LGTM!Import path correctly updated to reflect the package move of
Paramsto theinnerpackage.balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala (1)
8-21: LGTM!The
PostgresObjectQueryParamValueimplementation correctly extendsQueryParamValueand efficiently creates thePGobjectonce during construction. TheassignFuncproperly binds the object to thePreparedStatement.balta/src/test/scala/za/co/absa/db/balta/implicits/PostgresRowIntegrationTests.scala (1)
21-22: LGTM!Import paths correctly updated to reflect the new package structure for
PostgresRowandSimpleJsonStringunder thepostgrespackage.balta/src/main/scala/za/co/absa/db/balta/postgres/implicits/Postgres.scala (2)
17-21: LGTM!Package path and import correctly updated to align with the new Postgres-specific package structure.
23-34: LGTM!The
PostgresRowimplicit class correctly provides extension methods for retrievingSimpleJsonStringvalues from query results. The removal of deprecatedJsonBStringsupport aligns with the PR objectives.balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
27-40: LGTM!The refactored
runQuerymethod correctly iterates overQueryParamValueinstances, binding parameters viaassignwhen present and skipping index increment forNullParamValue(whereassignisNone). ThefoldLeftpattern cleanly manages the parameter index.balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (2)
17-22: LGTM!Package and imports correctly updated to reflect the Postgres-specific location and new typeclass infrastructure.
31-37: LGTM!The companion object provides a clean typeclass implementation for
SimpleJsonString. The implicitQueryParamType[SimpleJsonString]correctly delegates toPostgresObjectQueryParamValuewith the"json"PostgreSQL type, enabling seamless parameter binding.balta/src/main/scala/za/co/absa/db/balta/classes/DBConnection.scala (2)
19-20: LGTM on imports.The imports are correctly updated to reference
ConnectionInfofrom the newinnerpackage, andimplicitConversionsis appropriately imported for the new implicit conversion.
37-40: LGTM on implicit conversion.The implicit conversion enables seamless use of
DBConnectionwherever a JDBCConnectionis expected. This aligns with the PR's goal of improving ergonomics and replaces the removed implicit frombalta/implicits/package.scala.balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (4)
44-46: LGTM on insert method updates.The method correctly uses
values.values.map(_.sqlEntry)to build the SQL parameter placeholders and passesvalues.values(theList[QueryParamValue]) torunQuery. This aligns with the new value-based binding approach.
61-61: LGTM on fieldValue signature change.The context bound change from
AllowedParamTypestoQueryParamTypecorrectly adopts the new typeclass-based constraint, enabling proper implicit resolution for key values.
217-223: LGTM on private compose methods.The signature updates to accept
List[QueryParamValue]with a default empty list are consistent across all three methods, and the integration withrunQueryis correct.Also applies to: 226-231, 234-240
251-256: LGTM on WHERE condition generation.The use of
value.equalityOperator(which returns"IS"for NULL and"="otherwise) andvalue.sqlEntry(which returns"NULL"or"?") correctly handles both regular values and NULLs in WHERE clauses.balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (3)
28-30: Clean typeclass design.The
QueryParamType[T]trait with a singletoQueryParamValuemethod is appropriately minimal and focused. This design enables extensibility for custom types (like the Postgres-specificSimpleJsonStringshown in the snippets).
34-84: Good coverage of common SQL types.The implicit instances cover the most commonly used SQL parameter types. The conversions are appropriate:
Instant→OffsetDateTimeat UTC is a safe defaultBigDecimal→java.math.BigDecimalvia.bigDecimalis correctChar→Stringvia.toStringhandles single-character values properly
86-90: Elegant NULL representation.Using a singleton
NULLobject with its own typeclass instance is a clean pattern that integrates well with theadd(QueryParamType.NULL)API, replacing the deprecatedaddNullmethods.balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (3)
22-26: Well-designed abstraction.The
QueryParamValuetrait cleanly encapsulates three concerns:
- How to bind the value (
assign)- What to emit in SQL (
sqlEntry)- How to compare for equality in WHERE clauses (
equalityOperator)The defaults (
"?"and"=") work for most cases while allowingNullParamValueto override appropriately.
31-40: LGTM on concrete implementations.
ObjectQueryParamValueandSimpleQueryParamValuecorrectly implement the trait withSome(assignFunc)for theirassignvalues. The privateassignFuncinObjectQueryParamValueproperly encapsulates the implementation detail.
42-46: Correct NULL semantics.
NullParamValuecorrectly:
- Returns
Noneforassign(no PreparedStatement binding needed)- Uses
"NULL"literal in SQL instead of a placeholder- Uses
"IS"for equality checks (SQL standard:column IS NULLnotcolumn = NULL)balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (1)
123-125: LGTM on add method updates.The context bound change from
AllowedParamTypestoQueryParamTypeis correct and consistent with the typeclass refactoring throughout the codebase.Also applies to: 145-147
balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (4)
22-22: LGTM: Import updated for typeclass refactoring.The import correctly brings in the new
QueryParamTypeandQueryParamValuetypes that replace the oldAllowedParamTypesandSetterFnc.
106-110: LGTM: Typeclass pattern correctly implemented.The
setParammethod properly uses theQueryParamTypecontext bound and converts values through the typeclass interface. The implementation is type-safe and consistent with the refactoring goals.
137-137: LGTM: Type alias correctly updated.The
ParamsMaptype alias now usesQueryParamValueinstead ofSetterFnc, which is consistent with the typeclass-based refactoring.
166-170: LGTM: Positioned parameter handling correctly refactored.The
setParammethod for positioned parameters correctly implements the typeclass pattern, consistent with the named parameter version.balta/src/main/scala/za/co/absa/db/balta/classes/inner/Params.scala (9)
17-17: LGTM: Package reorganization.The move to the
innerpackage reflects the architectural reorganization mentioned in the PR objectives.
20-20: LGTM: Imports updated correctly.The import statement properly brings in the new typeclass types.
28-36: LGTM: Base class correctly refactored.The
Paramsabstract class properly usesQueryParamValuethroughout, and the public API (values,apply) correctly reflects the new types.
50-63: LGTM: Named parameter factory methods correctly implemented.Both
addandaddNullfactory methods properly use the typeclass pattern. The delegation fromaddNulltoaddwithQueryParamType.NULLis appropriate.
73-75: LGTM: Ordered parameter factory method.The
addfactory method correctly uses theQueryParamTypecontext bound.
93-93: LGTM: NamedParams constructor updated.The constructor properly uses
QueryParamValuein the internal map.
102-105: LGTM: Type-safe parameter addition.The
addmethod correctly usesimplicitly[QueryParamType[T]].toQueryParamValue(value)to convert values through the typeclass, ensuring type safety.
119-119: LGTM: pairs method updated.The return type correctly reflects the new
QueryParamValuetype.
129-141: LGTM: OrderedParams implementation.The
OrderedParamsclass correctly implements parameter addition using the typeclass pattern. The key generation (items.size.toString) and value conversion are properly handled.project/build.properties (1)
1-1: Patch-level sbt upgrade looks good; just confirm your JDK matrix.Bumping from 1.11.5 to 1.11.7 keeps you within the same major/minor line and should be low-risk, especially given prior experience with sbt 1.x on JDK 8 in this org. Still, it’s worth confirming your oldest supported JDK passes CI with this version (and that all sbt plugins still resolve cleanly). Based on learnings, sbt 1.11.x is expected to continue running on JDK 8, but verification in your pipeline is recommended.
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala
Outdated
Show resolved
Hide resolved
|
|
||
| object NULL | ||
|
|
||
| implicit object SQLParamNull extends QueryParamType[NULL.type] { |
There was a problem hiding this comment.
what about the custom type? I remember that we use this on many places
There was a problem hiding this comment.
This should be now done via it's own type-class implementation.
There was a problem hiding this comment.
class MyType {
}
implicit object QueryParamMyClasse xtendes QueryParamType[MyClass] {
override def toQueryParamValue(value: MyClass): QueryParamValue = ???
}
There was a problem hiding this comment.
I gave it a read and I like the changes although some are not backward compatible - perhaps it can be part of the major version release v1.0.0 probably.
I asked a Q about Custom DB type and also the AI Agent is telling you about some unused imports (not sure if false positives?)
Also I think RNs check is failing because the format is off in PR description but that can be easily fixed
Yes, it has breaking changes. But IMHO this approach is one of the preconditions for v1.0.0 (probably the only one crucial, although I still might do the column names bug).
|
* unified implicit object names to adhere to trait name * removed the usage of obsolete functions in tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (1)
32-32: Consider the scope impact of the implicit conversion.The implicit conversion from
StringtoSimpleJsonStringprovides convenience but may lead to unintended conversions whenSimpleJsonStringis imported. Since this is a deliberate design choice for a value class and part of the v1.0.0 API, verify that the automatic conversion behavior aligns with your intended usage patterns.balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (1)
154-157: Verify deprecation consistency.The
addNull[T: QueryParamType]()method retains the type parameter constraint, but since it always returns a NULL value, the type parameter seems vestigial. Consider whether the signature should be simplified toaddNull()without the type parameter, matching the simpleradd(NULL)replacement pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala(5 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala(1 hunks)balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala(10 hunks)balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala(2 hunks)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala(1 hunks)balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- balta/src/test/scala/za/co/absa/db/balta/classes/DBTableIntegrationTests.scala
🧰 Additional context used
🧬 Code graph analysis (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (4)
QueryParamValue(28-48)SimpleQueryParamValue(38-40)ObjectQueryParamValue(31-36)NullParamValue(42-46)balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (1)
toQueryParamValue(35-35)balta/src/main/scala/za/co/absa/db/balta/implicits/OptionImplicits.scala (1)
value(37-37)
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (3)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (15)
QueryParamType(32-92)toQueryParamValue(35-35)toQueryParamValue(39-39)toQueryParamValue(43-43)toQueryParamValue(47-47)toQueryParamValue(51-51)toQueryParamValue(55-55)toQueryParamValue(59-59)toQueryParamValue(63-63)toQueryParamValue(67-67)toQueryParamValue(71-71)toQueryParamValue(75-75)toQueryParamValue(79-79)toQueryParamValue(83-83)toQueryParamValue(89-89)balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)balta/src/main/scala/za/co/absa/db/balta/postgres/typeclasses/PostgresQueryParamValue.scala (2)
PostgresQueryParamValue(8-21)PostgresObjectQueryParamValue(9-18)
balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (2)
balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamValue.scala (1)
QueryParamValue(28-48)balta/src/main/scala/za/co/absa/db/balta/classes/DBFunction.scala (1)
sql(37-46)
🔇 Additional comments (11)
balta/src/main/scala/za/co/absa/db/balta/postgres/classes/SimpleJsonString.scala (2)
17-22: Package structure and imports are correct.The package placement aligns with the Postgres-specific code organization, and all imports are actually used in the implementation below. Contrary to concerns mentioned in the PR comments about potentially unused imports, this file has no unused imports.
34-36: Typeclass implementation is correct.The
QueryParamType[SimpleJsonString]instance correctly follows the typeclass pattern and properly delegates toPostgresObjectQueryParamValuewith the "json" type, consistent with the repository's new parameter binding approach.balta/src/main/scala/za/co/absa/db/balta/classes/DBQuerySupport.scala (1)
31-38: LGTM! Correct handling of NULL parameter binding.The foldLeft logic properly distinguishes between bound parameters (
Some(assignFnc)) which require PreparedStatement binding and NULL values (None) which are embedded directly as SQL literals. The parameter index correctly advances only for bound values.balta/src/main/scala/za/co/absa/db/balta/typeclasses/QueryParamType.scala (3)
28-30: Clean typeclass design.The
QueryParamType[T]trait provides a well-structured abstraction for converting Scala/Java types to JDBC-compatible parameter values. This enables ad-hoc polymorphism and easy extensibility for custom types.
66-68: UTC zone hardcoded for Instant conversion.The
Instant→OffsetDateTimeconversion assumes UTC. This is a reasonable default for DB storage, but users needing a different timezone can explicitly convert toOffsetDateTimebefore passing.
86-90: Well-designed NULL representation.The
NULLsingleton object with its typeclass instance cleanly integrates SQL NULL semantics into the typeclass system. This enablesadd(QueryParamType.NULL)usage as recommended in the deprecation messages.balta/src/main/scala/za/co/absa/db/balta/DBTestSuite.scala (2)
122-124: Type constraint updated correctly.The migration from
AllowedParamTypestoQueryParamTypecontext bound aligns with the new typeclass-based parameter handling.
132-135: Deprecation annotation is properly informative.The deprecation message clearly directs users to the new
add(NULL)pattern and specifies the version when deprecated.balta/src/main/scala/za/co/absa/db/balta/classes/DBTable.scala (3)
19-21: Imports correctly updated.The imports now reference the new
inner.Paramsstructure and include bothQueryParamValueandQueryParamTypeas needed for the refactored implementation.
249-254: Correct SQL NULL comparison semantics.The
paramsToWhereConditionlogic properly handles NULL values by usingvalue.equalityOperator(which is"IS"for NULL) andvalue.sqlEntry(which is"NULL"for NULL), producing correct SQL likefieldName IS NULLinstead of the incorrectfieldName = NULL.
42-44: INSERT parameter binding looks correct.The
sqlEntryvalues (placeholders?for bound values, literalNULLfor nulls) are correctly interpolated into the SQL, andvalues.valuesis passed torunQueryfor PreparedStatement binding.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/check_pr_release_notes.yml (1)
17-17: Minor: Slightly redundant workflow and job naming.The workflow name contains "Release Notes Presence Check" and the job display name is also "Release Notes Presence Check," which is somewhat repetitive. Consider shortening the workflow name to something like "Check Release Notes Presence" or the job name to just "Check Release Notes" to reduce redundancy.
Also applies to: 25-26
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/check_pr_release_notes.yml(1 hunks)
🔇 Additional comments (3)
.github/workflows/check_pr_release_notes.yml (3)
17-41: Good: Migration to a dedicated external action improves maintainability.Replacing custom inline GitHub Script logic with a standardized external action is a solid move. This reduces maintenance burden and allows the team to focus on business logic rather than workflow orchestration. The change aligns well with the PR's broader shift toward modular, standardized approaches.
34-41: All parameters and version are correctly configured; no issues identified.The verification confirms:
- Action version:
AbsaOSS/release-notes-presence-check@v0.4.0exists and is actively maintained.- Parameters: All three parameters are correctly named according to the action's documentation:
github-repository✓pr-number✓skip-labels✓- Skip-label convention: The label
"no RN"is consistently used across the project's workflows (also appears inrelease_draft.ymlasskip-release-notes-label: "no RN"), confirming it aligns with established project conventions.The workflow configuration is accurate and follows the project's established patterns.
30-32: Based on my verification, I've found critical information about this workflow setup:Key Findings:
- The action
AbsaOSS/release-notes-presence-check@v0.4.0referenced in line 35 does not exist in public GitHub- A similar action
AbsaOSS/generate-release-notes@v0.4.0DOES exist and is Python-based (found inrelease_draft.ymlwith identical Python 3.11 setup)- No Python code execution occurs in the
check_pr_release_notes.ymlworkflow after the setup- The Python setup appears to have been copy-pasted from
release_draft.ymlbut may be unnecessary for the non-existent or private action being referenced
Verify that the external action
AbsaOSS/release-notes-presence-check@v0.4.0exists and requires Python 3.11.The referenced action does not appear in the GitHub Marketplace or public AbsaOSS repositories. A similarly named action (
AbsaOSS/generate-release-notes@v0.4.0) exists and is Python-based, suggesting the Python setup may have been incorrectly copied. If this action is private or the reference is incorrect, the Python setup step may be unnecessary and should be removed or the action reference clarified.
benedeki
deleted the
feature/52-use-type-classe-for-allowedparamtypes
branch
2 participants
AllowedParamTypesrenamed toQueryParamTypeand changed to typeclassSetterFncrenamed and refactored intoQueryParamValueJsonBStringclass and supportaddNullfunctions instead implemented a constantQueryParamValue.NULL.gitignoreto better cover VS Code filesCloses #52
Summary by CodeRabbit
New Features
Deprecations / Breaking
Chores / Refactor
✏️ Tip: You can customize this high-level summary in your review settings.
Release notes: