JetBrains / Exposed

Kotlin SQL Framework
http://jetbrains.github.io/Exposed/
Apache License 2.0
8.27k stars 690 forks source link

feat: EXPOSED-388 Support for column type converters #2143

Closed obabichevjb closed 2 months ago

obabichevjb commented 3 months ago

Here is the PR that allows to transform columns from their original types to another.

Transformations could be nested, and could be applied to nullable columns. Transformation of a non-nullable column produces a non-nullable column, and transformation of a nullable column produces a nullable one. Creating nullable columns requires not only types changing, but also set field nullable into true, so still the method nullable() must be used.

We already have transform() for columns on the DAO layer, so I took naming from there (toColumn, toReal methods):

.transform(
    toColumn: (TReal) -> TColumn,
    toReal: (TColumn) -> TReal
)

But probably it's not the perfect variant. I was initially thinking about Target/Source instead of TReal/TColumn respectively. If you prefer the first variant (or have other suggestions) I'd be happy to know. It would be breaking change, but it could be changed for DAO as well if needed.

obabichevjb commented 2 months ago

Updated PR. I tried to check how similar APIs look in other libraries and tried to make it more consistent inside Exposed.

At the current moment I stopped at the variant of type names for transformation as Source/Dest pair; we already have such namings across other methods, and it looks reasonable for generic transformation.

But for more specific cases there are changes:

Also in all the generic interfaces/methods the source/column type is the first. It was not aligned before.

One more change is the renaming of base transformation class for DSL into ColumnWithTransform, and for DAO into EntityFieldWithTransform.

obabichevjb commented 2 months ago

I would sum up the questions for the discussion:

Naming

The naming toSource/toTarget most chosen as a most generic to fit dsl column transform, entity column transform, and chained transformations. Another variant I had before (but finally changed) was toColumn/toTransformed, but it was looking a bit weird in the case of chained transformations.

So I made a generic transformation interface and specific parameters for specific cases. That's why transform() methods have toColumn/toTarget for the first call and toCurrentTarget/toNextTarget for the second one. But it looks like it's over complicated and not so meaningful.

The method name toColumn I chose by analogy with IColumnType::valueToDB, so the second variant I liked was to name it like in IColumnType, it could toColumn/fromColumn or valueToColumn/valueFromColumn. One more question in this case would be in the names of generic types. There are also several options (during implementation I wanted several times to write TTransformed but it looks weird..) like TTransformed,TransformedType,TransformedValue/TColumn,ColumnType,ColumnValue. TColumn is a good variant because we already use it in several places, but paired TTransformed looks weird.

From the comment above the variant wrap/unwrap looks interesting. It could be used with types TWrapped/TColumn.

DSL/DAO

I also do not see something blocking from removing DAO's transform. Anyway we could mark it as deprecated and check if somebody complains about the case that could not be covered with DSL's only variant.

Potential reasons to keep both variants I see:

obabichevjb commented 2 months ago

Updated PR.

Methods toTarget()/toColumn(), toTarget()/toSource() (and original toReal()/toColumn()) were renamed to wrap/unwrap. Generic types Source/Target, TColumn/TReal were renamed to Unwrapped/Wrapped