Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Timestamp fixes, uuid support #49

Merged
merged 2 commits into from Feb 23, 2022
Merged

Timestamp fixes, uuid support #49

merged 2 commits into from Feb 23, 2022

Conversation

ievgen-kapinos
Copy link
Contributor

Closes #48
Plus:

  • Added tests
  • Added UUID support
  • Improved existed tests
  • Added ability to configure Hikari
  • Deprecated HikariCP.default(...) because redundant
  • Updated dependency versions
  • Applied same style for not imported classes

seratch
seratch requested changes Feb 23, 2022
View reviewed changes
Copy link
Owner

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Overall, the changes look great but can you check my comments?

build.gradle Outdated
// TODO: upgrade to 5+ when dropping Java 8 supports
implementation 'com.zaxxer:HikariCP:4.0.3'
implementation "com.zaxxer:HikariCP:5.0.1"
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this major upgrade this time? We'll do this in the future but not now.

src/main/kotlin/kotliquery/HikariCP.kt Outdated
@@ -9,18 +9,36 @@ object HikariCP {

private val pools: ConcurrentMap<String, HikariDataSource> = ConcurrentHashMap()

fun default(url: String, username: String, password: String): HikariDataSource {
@Deprecated(
Copy link
Owner

Choose a reason for hiding this comment

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

Can you revert this? I'm not planning to make this deprecated.

src/main/kotlin/kotliquery/HikariCP.kt
url: String,
username: String,
password: String,
configPropertiesFn: HikariConfig.() -> Unit = { }
Copy link
Owner

Choose a reason for hiding this comment

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

Nice enhancement!

src/main/kotlin/kotliquery/Session.kt
@@ -19,7 +27,7 @@ open class Session(
open val autoGeneratedKeys: List<String> = listOf(),
var transactional: Boolean = false,
open val strict: Boolean = false
) : AutoCloseable {
) : Closeable {
Copy link
Owner

Choose a reason for hiding this comment

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

This is fine but can you share the reason why I've changed this interface to sub interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allows to utilize use function build-in Kotlin

https://stackoverflow.com/a/46843631/5740547

If you take a look in its source code it is way reacher than LoanPattern. Check out the source code, not a copy-paste in stackoverflow.

src/main/kotlin/kotliquery/Session.kt
is org.joda.time.LocalTime -> this.setTime(idx, java.sql.Time(v.toDateTimeToday().millis))
is java.util.Date -> this.setTimestamp(idx, Timestamp(v.time))
is java.sql.Timestamp -> this.setTimestamp(idx, v)
is java.sql.Timestamp -> this.setTimestamp(idx, v) // extends java.util.Date
Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for fixing the order here

seratch
seratch approved these changes Feb 23, 2022
View reviewed changes
Copy link
Owner

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM. Will release a new version shortly

@seratch seratch merged commit e9ef78d into seratch:master Feb 23, 2022
@ievgen-kapinos
Copy link
Contributor Author

Thanks, LGTM. Will release a new version shortly

Recently I've revisited Spring's JdbcTemplate. So much history in it's implementation, so many layers, so deep stacktraces Your lib is totally different. Well done!

Waiting for new version! Have a nice day!

@seratch seratch added the bug label Feb 24, 2022
@ievgen-kapinos ievgen-kapinos deleted the timestamps_uuid branch February 24, 2022 08:15
@busches
Copy link

busches commented Feb 24, 2022

On moving from 1.6.1 to 1.6.2 we now get the following:

org.postgresql.util.PSQLException: ERROR: operator does not exist: uuid = character varying
      Hint: No operator matches the given name and argument types. You might need to add explicit type casts.
      Position: 773
        at app//org.postgresql.core.v3.QueryExecutorImpl.receiveErrorResponse(QueryExecutorImpl.java:2675)
        at app//org.postgresql.core.v3.QueryExecutorImpl.processResults(QueryExecutorImpl.java:2365)
        at app//org.postgresql.core.v3.QueryExecutorImpl.execute(QueryExecutorImpl.java:355)
        at app//org.postgresql.jdbc.PgStatement.executeInternal(PgStatement.java:490)
        at app//org.postgresql.jdbc.PgStatement.execute(PgStatement.java:408)
        at app//org.postgresql.jdbc.PgPreparedStatement.executeWithFlags(PgPreparedStatement.java:166)
        at app//org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:118)

The query is using a UUID type variable into a WHERE clause. I would not have expected a patch release to result in a breaking change.

@ievgen-kapinos
Copy link
Contributor Author

@busches Confirm it. I'll make a fix soon

@ievgen-kapinos ievgen-kapinos mentioned this pull request Feb 24, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seratch seratch seratch approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

java.time.Instant stored without nanoseconds
3 participants
@ievgen-kapinos @busches @seratch