Skip to content

Make parameters classes easier to work with #509

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Make parameters classes easier to work with #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 4 additions & 2 deletions client-base/src/main/kotlin/app/cash/backfila/client/spi/BackfillRegistration.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ package app.cash.backfila.client.spi
import java.time.Instant
import kotlin.reflect.KClass

data class BackfillRegistration @JvmOverloads constructor(
data class BackfillRegistration
@JvmOverloads
constructor(
val name: String,
val description: String?,
val parametersClass: KClass<Any>,
val parametersClass: KClass<out Any>,
val deleteBy: Instant?,
val unit: String? = null,
)
48 changes: 26 additions & 22 deletions client-base/src/test/kotlin/app/cash/backfila/client/fixedset/FixedSetBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,13 @@ import kotlin.reflect.KClass
import kotlin.reflect.full.findAnnotation

@Singleton
class FixedSetBackend @Inject constructor(
class FixedSetBackend
@Inject
constructor(
private val injector: Injector,
@ForFixedSetBackend private val backfills: MutableMap<String, KClass<out FixedSetBackfill<*>>>,
private val datastore: FixedSetDatastore,
) : BackfillBackend {

/** Creates Backfill instances. Each backfill ID gets a new Backfill instance. */
private fun getBackfill(name: String): FixedSetBackfill<*>? {
val backfillClass = backfills[name]
Expand All @@ -34,13 +35,16 @@ class FixedSetBackend @Inject constructor(
}
}

private fun <Param : Any> createOperator(
backfill: FixedSetBackfill<Param>,
) = FixedSetBackfillOperator(
backfill = backfill,
datastore = datastore,
parametersOperator = BackfilaParametersOperator(parametersClass(backfill::class)),
)
@Suppress("UNCHECKED_CAST")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this? I don't know that I like casting the operator, I'd rather cast the Param type exclusively.

Copy link
Author

Choose a reason for hiding this comment

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

Before we were always creating this with a parameters class of Any. Here we are casting to the actual parameters type

private fun <Param : Any> createOperator(backfill: FixedSetBackfill<Param>) =
FixedSetBackfillOperator(
backfill = backfill,
datastore = datastore,
parametersOperator =
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
)

override fun create(backfillName: String): BackfillOperator? {
val backfill = getBackfill(backfillName)
Expand All @@ -53,26 +57,26 @@ class FixedSetBackend @Inject constructor(
return null
}

override fun backfills(): Set<BackfillRegistration> {
return backfills.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<FixedSetBackfill<Any>>),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.ITEMS.displayName,
)
}.toSet()
}
override fun backfills(): Set<BackfillRegistration> =
backfills
.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.ITEMS.displayName,
)
}.toSet()

private fun <T : Any> parametersClass(backfillClass: KClass<out FixedSetBackfill<T>>): KClass<T> {
private fun parametersClass(backfillClass: KClass<out FixedSetBackfill<*>>): KClass<*> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I don't think that removing this anchoring to the type is what I'm looking for.

I don't want parameters to be able to be any type for a particular backfill but only the parameterized type for that specific backfill.

Copy link
Author

Choose a reason for hiding this comment

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

You still get that here. Try it for yourself, the types still have to match up

// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like Backfill<MyDataClass>.
val supertype = thisType.getSupertype(FixedSetBackfill::class.java).type as ParameterizedType

// Like MyDataClass
return (Types.getRawType(supertype.actualTypeArguments[0]) as Class<T>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[0])).kotlin
}
}
49 changes: 26 additions & 23 deletions ...namodb-v2/src/main/kotlin/app/cash/backfila/client/dynamodbv2/internal/DynamoDbBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import kotlin.reflect.full.findAnnotation
import software.amazon.awssdk.services.dynamodb.DynamoDbClient

@Singleton
class DynamoDbBackend @Inject constructor(
class DynamoDbBackend
@Inject
constructor(
private val injector: Injector,
@ForDynamoDbBackend private val backfills: MutableMap<String, KClass<out DynamoDbBackfill<*, *>>>,
private val dynamoDbClient: DynamoDbClient,
val keyRangeCodec: DynamoDbKeyRangeCodec,
) : BackfillBackend {

/** Creates Backfill instances. Each backfill ID gets a new Backfill instance. */
private fun getBackfill(name: String): DynamoDbBackfill<*, *>? {
val backfillClass = backfills[name]
Expand All @@ -38,14 +39,16 @@ class DynamoDbBackend @Inject constructor(
}
}

private fun <E : Any, Param : Any> createDynamoDbOperator(
backfill: DynamoDbBackfill<E, Param>,
) = DynamoDbBackfillOperator(
dynamoDbClient,
backfill,
BackfilaParametersOperator(parametersClass(backfill::class)),
keyRangeCodec,
)
@Suppress("UNCHECKED_CAST")
private fun <E : Any, Param : Any> createDynamoDbOperator(backfill: DynamoDbBackfill<E, Param>) =
DynamoDbBackfillOperator(
dynamoDbClient,
backfill,
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
keyRangeCodec,
)

override fun create(backfillName: String): BackfillOperator? {
val backfill = getBackfill(backfillName)
Expand All @@ -58,26 +61,26 @@ class DynamoDbBackend @Inject constructor(
return null
}

override fun backfills(): Set<BackfillRegistration> {
return backfills.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<DynamoDbBackfill<Any, Any>>),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.SEGMENTS.displayName,
)
}.toSet()
}
override fun backfills(): Set<BackfillRegistration> =
backfills
.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.SEGMENTS.displayName,
)
}.toSet()

private fun <P : Any> parametersClass(backfillClass: KClass<out DynamoDbBackfill<*, P>>): KClass<P> {
private fun parametersClass(backfillClass: KClass<out DynamoDbBackfill<*, *>>): KClass<*> {
// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like DynamoDbBackfill<MyItemClass, MyParameterClass>.
val supertype = thisType.getSupertype(DynamoDbBackfill::class.java).type as ParameterizedType

// Like MyParameterClass
return (Types.getRawType(supertype.actualTypeArguments[1]) as Class<P>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[1])).kotlin
}
}
49 changes: 26 additions & 23 deletions ...nt-dynamodb/src/main/kotlin/app/cash/backfila/client/dynamodb/internal/DynamoDbBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,14 @@ import kotlin.reflect.KClass
import kotlin.reflect.full.findAnnotation

@Singleton
class DynamoDbBackend @Inject constructor(
class DynamoDbBackend
@Inject
constructor(
private val injector: Injector,
@ForDynamoDbBackend private val backfills: MutableMap<String, KClass<out DynamoDbBackfill<*, *>>>,
val dynamoDb: DynamoDBMapper,
val keyRangeCodec: DynamoDbKeyRangeCodec,
) : BackfillBackend {

/** Creates Backfill instances. Each backfill ID gets a new Backfill instance. */
private fun getBackfill(name: String): DynamoDbBackfill<*, *>? {
val backfillClass = backfills[name]
Expand All @@ -38,14 +39,16 @@ class DynamoDbBackend @Inject constructor(
}
}

private fun <E : Any, Param : Any> createDynamoDbOperator(
backfill: DynamoDbBackfill<E, Param>,
) = DynamoDbBackfillOperator(
dynamoDb,
backfill,
BackfilaParametersOperator(parametersClass(backfill::class)),
keyRangeCodec,
)
@Suppress("UNCHECKED_CAST")
private fun <E : Any, Param : Any> createDynamoDbOperator(backfill: DynamoDbBackfill<E, Param>) =
DynamoDbBackfillOperator(
dynamoDb,
backfill,
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
keyRangeCodec,
)

override fun create(backfillName: String): BackfillOperator? {
val backfill = getBackfill(backfillName)
Expand All @@ -58,26 +61,26 @@ class DynamoDbBackend @Inject constructor(
return null
}

override fun backfills(): Set<BackfillRegistration> {
return backfills.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<DynamoDbBackfill<Any, Any>>),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.SEGMENTS.displayName,
)
}.toSet()
}
override fun backfills(): Set<BackfillRegistration> =
backfills
.map {
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.SEGMENTS.displayName,
)
}.toSet()

private fun <P : Any> parametersClass(backfillClass: KClass<out DynamoDbBackfill<*, P>>): KClass<P> {
private fun parametersClass(backfillClass: KClass<out DynamoDbBackfill<*, *>>): KClass<*> {
// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like DynamoDbBackfill<MyItemClass, MyParameterClass>.
val supertype = thisType.getSupertype(DynamoDbBackfill::class.java).type as ParameterizedType

// Like MyParameterClass
return (Types.getRawType(supertype.actualTypeArguments[1]) as Class<P>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[1])).kotlin
}
}
11 changes: 7 additions & 4 deletions client-jooq/src/main/kotlin/app/cash/backfila/client/jooq/internal/JooqBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,21 @@ class JooqBackend @Inject constructor(
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<JooqBackfill<*, Any>>),
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.ITEMS.displayName,
)
}.toSet()
}

@Suppress("UNCHECKED_CAST")
private fun <K : Any, Param : Any> createJooqOperator(
backfill: JooqBackfill<K, Param>,
) = JooqBackfillOperator(
backfill,
BackfilaParametersOperator(parametersClass(backfill::class)),
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
)

/** Creates Backfill instances. Each backfill ID gets a new Backfill instance. */
Expand All @@ -60,14 +63,14 @@ class JooqBackend @Inject constructor(
}
}

private fun <T : Any> parametersClass(backfillClass: KClass<out JooqBackfill<*, T>>): KClass<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does removing the generic mean the type isn't known at the callsite?

Copy link
Author

Choose a reason for hiding this comment

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

Before, we were always casting the type to Any so that it was "known" at the callsite. This was all to satisfy the type of KClass<Any> on BackfillRegistration.parametersClass. Using the type KClass<Any> meant you couldn't use any params type without casting it to Any first

private fun parametersClass(backfillClass: KClass<out JooqBackfill<*, *>>): KClass<*> {
// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like Backfill<Key, MyDataClass>.
val supertype = thisType.getSupertype(JooqBackfill::class.java).type as ParameterizedType

// Like MyDataClass
return (Types.getRawType(supertype.actualTypeArguments[1]) as Class<T>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[1])).kotlin
}
}
11 changes: 7 additions & 4 deletions ...nate/src/main/kotlin/app/cash/backfila/client/misk/hibernate/internal/HibernateBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,14 @@ internal class HibernateBackend @Inject constructor(
return injector.getInstance(backfillClass.java) as HibernateBackfill<*, *, *>
}

@Suppress("UNCHECKED_CAST")
private fun <E : DbEntity<E>, Pkey : Any, Param : Any> createHibernateOperator(
backfill: HibernateBackfill<E, Pkey, Param>,
) = HibernateBackfillOperator(
backfill,
BackfilaParametersOperator(parametersClass(backfill::class)),
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
this,
)

Expand All @@ -56,22 +59,22 @@ internal class HibernateBackend @Inject constructor(
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<HibernateBackfill<*, *, Any>>),
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.ITEMS.displayName,
)
}.toSet()
}

private fun <T : Any> parametersClass(backfillClass: KClass<out HibernateBackfill<*, *, T>>): KClass<T> {
private fun parametersClass(backfillClass: KClass<out HibernateBackfill<*, *, *>>): KClass<*> {
// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like Backfill<E, Id<E>, MyDataClass>.
val supertype = thisType.getSupertype(HibernateBackfill::class.java).type as ParameterizedType

// Like MyDataClass
return (Types.getRawType(supertype.actualTypeArguments[2]) as Class<T>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[2])).kotlin
}

/** This placeholder exists so we can create a backfill without a type parameter. */
Expand Down
14 changes: 8 additions & 6 deletions client-s3/src/main/kotlin/app/cash/backfila/client/s3/internal/S3DatasourceBackend.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,20 @@ class S3DatasourceBackend @Inject constructor(
}
}

@Suppress("UNCHECKED_CAST")
private fun <R : Any, Param : Any> createS3DatasourceOperator(
backfill: S3DatasourceBackfill<R, Param>,
) = S3DatasourceBackfillOperator(
backfill,
BackfilaParametersOperator(parametersClass(backfill::class)),
BackfilaParametersOperator(
parametersClass(backfill::class),
) as BackfilaParametersOperator<Param>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we end up needing to cast here and didn't before?

Copy link
Author

Choose a reason for hiding this comment

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

We were casting to S3DatasourceBackfill<Any, Any> in create before (just below here). This allowed the Params type to always come through as KClass<Any> to satisfy the previous type of BackfillRegistration.parametersClass

s3Service,
)

override fun create(backfillName: String): BackfillOperator? {
return getBackfill(backfillName)?.let { backfill ->
@Suppress("UNCHECKED_CAST") // We don't know the types statically, so fake them.
return createS3DatasourceOperator(backfill as S3DatasourceBackfill<Any, Any>)
return createS3DatasourceOperator(backfill)
}
}

Expand All @@ -55,21 +57,21 @@ class S3DatasourceBackend @Inject constructor(
BackfillRegistration(
name = it.key,
description = it.value.findAnnotation<Description>()?.text,
parametersClass = parametersClass(it.value as KClass<S3DatasourceBackfill<Any, Any>>),
parametersClass = parametersClass(it.value),
deleteBy = it.value.findAnnotation<DeleteBy>()?.parseDeleteByDate(),
unit = BackfillUnit.BYTES.displayName,
)
}.toSet()
}

private fun <R : Any, P : Any> parametersClass(backfillClass: KClass<out S3DatasourceBackfill<R, P>>): KClass<P> {
private fun parametersClass(backfillClass: KClass<out S3DatasourceBackfill<*, *>>): KClass<*> {
// Like MyBackfill.
val thisType = TypeLiteral.get(backfillClass.java)

// Like S3DatasourceBackfill<MyParameterClass>.
val supertype = thisType.getSupertype(S3DatasourceBackfill::class.java).type as ParameterizedType

// Like MyParameterClass
return (Types.getRawType(supertype.actualTypeArguments[1]) as Class<P>).kotlin
return (Types.getRawType(supertype.actualTypeArguments[1])).kotlin
}
}
Loading