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.

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

fix: add traces to updates with generared keys #112

Merged

Conversation

ajkaanbal
Copy link
Contributor

I was having trouble tracing updates using withUniqueGeneratedKeys, these changes appear to have solved the problem so far.

@@ -14,7 +14,8 @@ import scala.annotation.nowarn
@nowarn
private[doobie] case class TracedStatement(
p: PreparedStatement,
queryString: String
queryString: String,
c: Any*
Copy link
Contributor

Choose a reason for hiding this comment

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

What does c stand for? Could you please use a more descriptive name?

I see that we didn't set a good example with p for preparedStatement on line 16, but we should do better in the future.

@voidcontext voidcontext self-assigned this Dec 14, 2023
Comment on lines +103 to +112
override def prepareStatement( a: String, b: Array[String]): Kleisli[F, Connection, PreparedStatement] =
super.prepareStatement(a, b).map(TracedStatement(_, a, b): PreparedStatement)
override def prepareStatement(a: String, b: Array[Int]): Kleisli[F, Connection, PreparedStatement] =
super.prepareStatement(a, b).map(TracedStatement(_, a, b): PreparedStatement)
override def prepareStatement(a: String, b: Int) =
super.prepareStatement(a, b).map(TracedStatement(_, a, b): PreparedStatement)
override def prepareStatement(a: String, b: Int, c: Int) =
super.prepareStatement(a, b, c).map(TracedStatement(_, a, b, c): PreparedStatement)
override def prepareStatement(a: String, b: Int, c: Int, d: Int) =
super.prepareStatement(a, b, c, d).map(TracedStatement(_, a, b, c, d): PreparedStatement)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems the variadic parameter is not used anywhere, why is it needed to be set?

@voidcontext
Copy link
Contributor

@ajkaanbal Could you please describe the issue / symptom you were experiencing?

@richard-dennehy-ovo
Copy link

@voidcontext voidcontext merged commit 6158054 into ovotech:master Feb 9, 2024
2 checks passed
@voidcontext
Copy link
Contributor

HI @ajkaanbal and @richard-dennehy-ovo, I had some time today so I added the suggested changes in #114 and merged it, I'll tag a fix release soon, it should be available in maven later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants