-
Notifications
You must be signed in to change notification settings - Fork 618
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
Implementation for automatically registering sealed serializers in polymorphic modules #2201
base: dev
Are you sure you want to change the base?
Conversation
f548387
to
1aead70
Compare
Hi, thanks for the PR! Feature is indeed looks useful, although I'm a reluctant about changing the behavior of existing function (it didn't throw exception on sealed classes before, so it's not adding a completely new functionality). Have you though about other possibilities of doing this? Like adding new |
I'll update the pull request later when I have time. While this introduces "changed" behaviour the current (without this request) implementation is not actually valid. As sealed classes (and interfaces) are abstract they don't actually make valid serializers in the polymorphic case. For serialization the value with never be directly of the sealed type, for deserialization it will never create a sealed instance. The only challenge with changing existing behaviour is when existing code does register the base for polymorphic serialization, this could have 2 consequences:
As the current code doesn't (appear to) throw an exception in case sealed parent is registered it may be best to use a separate function name. I'll update this in the pull request. |
@sandwwraith At a glance, no. #1865 is a different use case.
I don't have the types available at compile time, thus I can't register them in a serial module as you would 'normally' do. My use case is a framework in which the app building on top of it links in additional types, through reflection, which extend from an interface defined in the framework. The framework then supports serializing aggregations of such extending classes. I haven't looked at whether I could somehow create a |
I've updated the pull request by a split out function (although the tests that were deleted - and no longer are did verify that passing a sealed type serializer should fail for subclass). |
d584677
to
dfae643
Compare
dfae643
to
2e509cd
Compare
@Whathecode The thing this pull request does is support the case where there is a polymorphic base type |
…aled children of a class for polymorphic serialization of a shared (non-sealed) type.
…avoid casting warnings). Use a map to allow faster lookup of serializers to class (to allow for overlapping hierarchies).
308c6f8
to
4bdff60
Compare
I've rebased this upon the current dev, and am happy to make needed changes. For clarity this version does not reuse the function name, but has a separate function instead (the original one would not work validly on sealed classes). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this does not completely solve #1865, it is a viable workaround, so I'm in favor of adding such function after addressing all the comments
private var defaultSerializerProvider: ((Base) -> SerializationStrategy<Base>?)? = null | ||
private var defaultDeserializerProvider: ((String?) -> DeserializationStrategy<Base>?)? = null | ||
|
||
|
||
/** | ||
* Registers the child serializers for the sealed [subclass] [serializer] in the resulting module under the [base class][Base]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation here does not mention that Base
has to be sealed.
Also, it would be more helpful if documentation contained the example in which scenario this function has to be used. I think #2199 has a good example with
interface Base
@Serializable
sealed interface Sub
serializersModule {
polymorphic(Base::class) {
subclassesOf<Sub>()
}
}
/** | ||
* Registers the child serializers for the sealed [subclass] [serializer] in the resulting module under the [base class][Base]. | ||
*/ | ||
public inline fun <reified T : Base> subclassesOf(): Unit = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All new functions should be marked with @ExperimentalSerializationApi
* Registers the subclasses of the given class as subclasses of the outer class. This currently requires `baseClass` | ||
* to be sealed. | ||
*/ | ||
public fun <T: Base> subclassesOf(serializer: KSerializer<T>) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After thinking about it for a while, I came to the idea that subclassesOf
is a misleading name. It implies that we can extract all subclasses for an arbitrary class, which we can't do — and as we all know, not every person will be reading documentation to find out that we are able to handle only sealed classes. So more appropriate name would be sealedSubclassesOf
or subclassesOfSealed
or something in that direction.
This is an implementation for #2199 and #1865. This implementation just modifies the
subclass
method in the builder. It does (internally) expose the registered children inSealedSerializer
. It is not possible to trigger this behaviour from manually written serializers (instead ofSealedSerializer
).This does also remove the prohibition of registering sealed classes, but note that it will register the children of the sealed class, but not the sealed parent itself. The code does allow for overlaps (by ignoring the registration in this case) to accommodate overlapping hierarchies (although this can be abused in some ways) - it does mean that sealed child serializers can be manually overridden by registering the sub-serializer before the sealed parent.