chore: Make Sql.Interpolation backwards compatible #514
Conversation
* used in other projects that we don't want to break
| @@ -1,8 +1,4 @@ | |||
| ProblemFilters.exclude[DirectMissingMethodProblem]("akka.persistence.r2dbc.R2dbcSettings.this") | |||
There was a problem hiding this comment.
the file should be latest release, 1.2.1, not 1.2.2
| * interpolation arguments `$` can be used. | ||
| */ | ||
| @InternalApi private[akka] implicit class InterpolationWithAdapter(val sc: StringContext)(implicit | ||
| adapter: QueryAdapter) |
There was a problem hiding this comment.
QueryAdapter is private[akka] and therefore I think the InterpolationWithAdapter should also be private[akka], otherwise we should also make QueryAdapter public (InternalStableApi)
Note that this is AnyRef because of two parameters, instead of AnyVal. I'm not sure this idea with the adapter is worth the indirect complexity it adds. Doesn't we override all sql for sqlserver anyway in the SqlServerDao classes? Those could use @ as the placeholder instead of ? and then we could change the ordinary fillInParameterNumbers to look for ? and @ and replace accordingly. I'll might try that in a separate pr.
There was a problem hiding this comment.
ok, we don't override all sql, which is good. I'll think a little more about this, but this PR can be merged seperately
There was a problem hiding this comment.
Correct, we do not override all sql statements, which is why this QueryAdaper was introduced.
There was a problem hiding this comment.
I leave it as this for now. Projections will still be able to use QueryAdapter since it's private[akka].
patriknw
force-pushed
the
wip-compatibility-patriknw
branch
from
226a9b5 to
6d30fdf
Compare
| val queryAdapter: QueryAdapter) { | ||
|
|
||
| // implicits that can be imported | ||
| object JournalImplicits { |
There was a problem hiding this comment.
Have not seen this very often, objects within a case class. But I guess it is fine. Alternative could be something based on inheritance or so, this this CodecSettings.this.journalPayloadCodec could be avoided.
There was a problem hiding this comment.
CodecSettings.this.journalPayloadCodec was just because I used the same names
There was a problem hiding this comment.
yeah, the object is only for name scoping
| @@ -44,6 +44,7 @@ private[r2dbc] class SqlServerDurableStateDao( | |||
| connectionFactory: ConnectionFactory, | |||
| dialect: Dialect)(implicit ec: ExecutionContext, system: ActorSystem[_]) | |||
| extends PostgresDurableStateDao(settings, connectionFactory, dialect) { | |||
| import settings.codecSettings.DurableStateImplicits._ | |||
There was a problem hiding this comment.
Interesting, this means imports within the parent class are not available in the sub-class.
There was a problem hiding this comment.
That was a side effect, but I think it's good to be more explicit about the implicits. Can be difficult to understand where they come from otherwise.
Follow up to #505
Sql.Interpolationis used in other projects that we don't want to break.Also a cleanup of the codec settings.