feat: Add typealiases for generate table def criteria queries#109
feat: Add typealiases for generate table def criteria queries#109micahbeech wants to merge 2 commits into
Conversation
56fe6b2 to
c9ef83e
Compare
QuinnB73
left a comment
There was a problem hiding this comment.
LGTM with one question
| import com.squareup.kotlinpoet.asClassName | ||
|
|
||
| /** | ||
| * Generates: `typealias DbBookProjectedCriteriaQuery<PROJECTION> = ProjectedTypeSafeCriteriaQuery<DbBook, DbBook, DbBookTableDef<DbBook>, PROJECTION>` |
There was a problem hiding this comment.
Should we maybe generate ProjectedTypeSafeCriteriaQuery<DbBook, *, DbBookTableDef<DbBook>, PROJECTION>?
I believe that would allow us to reuse the type alias for subqueries, which could be useful
There was a problem hiding this comment.
Confirmed that your change works the way I expected locally
There was a problem hiding this comment.
Actually, what do you think about having two type aliases for ProjectedCriteriaQuery? One that is for the standard query with the same T and SOURCE, and one for subqueries with a generic T. That way we still provide the convenience but don't lose any type information. For example,
typealias DbBookProjectedCriteriaQuery<PROJECTION> = ProjectedTypeSafeCriteriaQuery<DbBook, DbBook, DbBookTableDef<DbBook>, PROJECTION>
typealias DbBookProjectedCriteriaSubQuery<T, PROJECTION> = ProjectedTypeSafeCriteriaQuery<DbBook, T, YawnTableDef<DbBook, T>, PROJECTION>Thoughts @QuinnB73?
There was a problem hiding this comment.
Okay so a couple of things:
I had my test backwards, and the current implementation actually doesn't work the way I was thinking. I have a couple of solutions for what I was thinking about, but first let me explain why I'm pushing for this. My main goals are twofold here, I want us to ideally be able to define one extension function that can be used both when in a projection context and a projected subquery context, and I want the extension function to feel nice to write. With your proposed solution, we'd have to define two duplicate extension functions if we wanted to use it in both contexts.
I think we have two solutions to achieve what I'm going for here:
Solution 1
We gen the type alias:
typealias BookProjectedCriteriaQuery<PROJECTION> = ProjectedTypesafeCriteriaQuery<*, DbBook, *, PROJECTION>And users can write functions like
private fun BookProjectedCriteriaQuery<*>.myFun(): Boolean = trueand then use it in both contexts.
Solution 2
We gen the type alias:
typealias BookProjectedCriteriaQuery<SOURCE, PROJECTION> = ProjectedTypeSafeCriteriaQuery<SOURCE, Book, BookTableDef<SOURCE>, PROJECTION>And then users can write function with a specific SOURCE, or can they can choose to write one like the following that could be used in both contexts
private fun <T: BaseEntity<T>> BookProjectedCriteriaQuery<T, *>.myFun(): Boolean = trueBoth solutions meet my first criteria, but I like the ergonomics of Solution 1 better. I've confirmed both typealiases work this way locally properly this time, btw 😅
Wrt your point about losing type information, I don't think we're losing anything we care about by using the star projection for these type args at this level. Internally Yawn uses SOURCE to enforce that all parts of a query come from the same root which allows us to guarantee that all restrictions/joins/projections etc. come from the same root. For a projected subquery, we've explicitly created a new query with a new root, but that root remains consistent throughout that query. As a consumer of Yawn writing reusable extensible functions, you don't really care what the root is. Yawn handles that for you. You do care that you have a YawnTableDef, but that's already guaranteed to you by the ProjectedTypeSafeCriteriaQuery type.
Let's please align on this prior to merge!
cc @luanpotter I want to make sure you're aligned with my thinking here and that my understanding of these type args is accurate
There was a problem hiding this comment.
honestly I've many many times thought of just killing SOURCE. it provides some extra protection but that a sufficiently smart person can easily bypass. so it is adding a lot of complexity for "naught"
I didn't go into the details but overall, the detached criterias are inherently from a different source so I think erasing that in that case makes total sense
a0b18e7 to
3b7dfdb
Compare
| import com.squareup.kotlinpoet.asClassName | ||
|
|
||
| /** | ||
| * Generates: `typealias DbBookCriteriaQuery = TypeSafeCriteriaQuery<DbBook, DbBookTableDef<DbBook>>` |
There was a problem hiding this comment.
would it be cleaner to have this be constructed using the previous simpler ones (like DbBookTableDefType)?
or is that just making it more convoluted?
(honestly not sure just something that crossed my mind)
There was a problem hiding this comment.
Depends how deep the nesting goes. As written, I'd expect this is a bit easier to reason, if we leverage the type aliases across the generated code then I would say we should use the aliases.
There was a problem hiding this comment.
Yeah I thought about this too. I lean towards not nesting the aliases to make it more grokkable for someone trying to determine what resolved type the typealias is aliasing. I think it'll be generally simpler to reason about and easier to maintain if we still with referencing the base types throughout the generated code, and only provide the typealiases for convenience to the end user
There was a problem hiding this comment.
Agreed with this fully! IMO the typealiases should just be convenience, Yawn should use its types directly internally for clarity
| import kotlin.reflect.KVisibility | ||
| import kotlin.reflect.typeOf | ||
|
|
||
| internal class YawnEntityProcessorTypeAliasesTest { |
There was a problem hiding this comment.
could we also add a very simple test on yawn-database-test that uses each generated type within a helper function in a query? more of a "documentation" and "double-checking" that these particular type aliases we chose are useful and how each can be expected to be used
Nava2
left a comment
There was a problem hiding this comment.
Should we have tests that validate we can use the aliases? Should we follow-up to this replacing usages of the unaliased with the aliased in the generated code?
| import com.squareup.kotlinpoet.asClassName | ||
|
|
||
| /** | ||
| * Generates: `typealias DbBookCriteriaQuery = TypeSafeCriteriaQuery<DbBook, DbBookTableDef<DbBook>>` |
There was a problem hiding this comment.
Depends how deep the nesting goes. As written, I'd expect this is a bit easier to reason, if we leverage the type aliases across the generated code then I would say we should use the aliases.
| val generators = listOf( | ||
| TableDefTypeAliasGenerator, | ||
| TypeSafeCriteriaQueryTypeAliasGenerator, | ||
| ProjectedTypeSafeCriteriaQueryTypeAliasGenerator, | ||
| JoinTypeSafeCriteriaQueryTypeAliasGenerator, | ||
| ) |
There was a problem hiding this comment.
nit: This list should be static or a field defintion given it doesn't change and isn't based on the yawnContext.
| for (typeAlias in typeAliases) { | ||
| addTypeAlias(typeAlias) | ||
| } | ||
| return this |
There was a problem hiding this comment.
| for (typeAlias in typeAliases) { | |
| addTypeAlias(typeAlias) | |
| } | |
| return this | |
| return typeAliases.fold(this, (that, typeAlias) -> that.addTypeAlias(typeAlias)) |
though, not sure that's actually cleaner 😓
There was a problem hiding this comment.
Personally I find the for loop a little easier to grok so gonna leave as-is, lmk if you feel strongly though
There was a problem hiding this comment.
Yeah I find this confusing but it might be because my Kotlin brain is too small
|
LGTM, thanks for updating the projection typealias to work in subquery contexts! Though +1 on adding tests |

Changes
Add type aliases for generated TableDef classes to simplify usage of Yawn's type-safe criteria query APIs.
Type Aliases Added
For each entity annotated with
@YawnEntity, the following type aliases are now generated:{Entity}TableDefType- Alias for{Entity}TableDef<{Entity}>{Entity}CriteriaQuery- Alias forTypeSafeCriteriaQuery<{Entity}, {Entity}TableDef<{Entity}>>{Entity}JoinCriteriaQuery- Alias forJoinTypeSafeCriteriaQuery<{Entity}, {Entity}, {Entity}TableDef<{Entity}>>{Entity}ProjectedCriteriaQuery<PROJECTION>- Alias forProjectedTypeSafeCriteriaQuery<{Entity}, {Entity}, {Entity}TableDef<{Entity}>, PROJECTION>Usage
The goal is to make referencing these types easier, in particular when writing criteria extension functions. For example: