Skip to content
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

Support encode/decode java.time.Duration and ConfigMemorySize in HOCON #2094

Merged
merged 2 commits into from
Feb 23, 2023

Conversation

alexmihailov
Copy link
Contributor

  • Many use cases require serialization support for java.time.Duration. It is not possible to implement a custom serializer in user code, since private methods from kotlinx.serialization.hocon.Hocon and kotlinx.serialization.hocon.AbstractHoconEncoder are required. To do this, I implemented the JDurationSerializer serializer, which can be used in user code.

  • Hocon supports the memory size format. Documentation.
    In the com.typesafe.config library, the ConfigMemorySize class is responsible for this functionality. There is no way in user code to implement a serializer for this class. So I've implemented a ConfigMemorySizeSerializer that allows to serialize ConfigMemorySize.

  • The com.typesafe.config library can create configuration using Java Bean notation. The com.typesafe.config.ConfigBeanFactory.create method is used for this. To support this functionality, I've implemented a JBeanSerializer that can only be used to decode configuration into a Java Bean class.

This PR depends on the previous PR

@ihostage
Copy link

ihostage commented Nov 9, 2022

Hi, Leonid @sandwwraith! 👋
FYI, this PR very useful to migrate from directly using Typesafe Config to Kotlinx.Serialization, because until now Kotlinx.Serialization don't have a few built-in Typesafe Config functionality 🤷‍♂️
Thanks, Alex @AlexMihaylov! 👍

@sandwwraith sandwwraith self-requested a review November 9, 2022 12:37
@alexmihailov alexmihailov changed the title Support encode/decode java.time.Duration and ConfigMemorySize, decode java objects in Java Bean notation. Support encode/decode java.time.Duration and ConfigMemorySize, decode java objects in Java Bean notation in HOCON Dec 26, 2022
@sandwwraith
Copy link
Member

Hi! I've scanned through this and your other PRs. It seems the key problem here is, as you said, It is not possible to implement a custom serializer in user code, since private methods from kotlinx.serialization.hocon.Hocon and kotlinx.serialization.hocon.AbstractHoconEncoder are required. — for all the serializers you propose. All of them share the same piece of code:

when (decoder) {
            is Hocon.ConfigReader -> decoder.getValueFromTaggedConfig(decoder.getCurrentTag(), valueResolver)
            is Hocon.ListConfigReader -> decoder.getValueFromTaggedConfig(decoder.getCurrentTag(), valueResolver)
            is Hocon.MapConfigReader -> decoder.getValueFromTaggedConfig(decoder.getCurrentTag(), valueResolver)
            else -> throw UnsupportedFormatException("ConfigMemorySizeSerializer")
        }

Which uses internal classes and internal function getValueFromTaggedConfig. I suggest making some public API out of it. You can take interface JsonDecoder as an inspiration: it allows to hook into the deserialization process and the raw value — exactly what is needed here. So you can introduce some HoconDecoder that all readers would implement and some function to get current value, like this (names are up to later consideration):

if (decoder is HoconDecoder) 
  (config, path) = decoder.currentValue() 
else
  error("Can be decoded only by Hocon")

I'm unsure if both config and path are needed or if one is enough. Also, maybe the current lambda valueResolver is better than returning a pair — it's up to you to find out. But I'm sure by making this API it would be possible to uncouple serializers from internal Hocon implementation details. Also, it would enable users to work with various classes, such as Java Duration, without a need to add their serializers to kotlinx-serialization-hocon library or without a need to wait for a new release. It'll allow keeping changes small and adding one serializer at a time if it is really need (for now, I think maybe it's better to left them outside this library, but I'm up to discussion).

@alexmihailov
Copy link
Contributor Author

Hi, Leonid @sandwwraith !
I have implemented a public api: HoconEncoder, HoconDecoder. It really does look a lot better and expands the possibilities for writing custom serializers.
In many cases java.time.Duration is used, so I would like to have a serializer for it in the library.
ConfigMemorySize is also often used. I also want the serializer for it to be in the library.
Java Bean notation is rarely used and the serializer for it can be removed from the library.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

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

(i haven't reviewed the tests yet)


/**
* Serializer for [ConfigMemorySize].
* For decode using [HOCON Size in bytes format](https://github.com/lightbend/config/blob/main/HOCON.md#size-in-bytes-format).
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry, I can't quite understand what you mean here. Are you saying decoding accepts all possible values from the spec (including powers of two and ten)? And encoding always emits powers of two?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, when decoding, can use all formats from the specification. For encoding, I selected on one option - to use powers of two, as it is easier to implement.

Copy link
Member

Choose a reason for hiding this comment

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

Then please rewrite the documentation as in JavaDurationSerializer

Copy link
Member

Choose a reason for hiding this comment

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

Bump: Rewrite as 'All possible Hocon size formats....'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@alexmihailov alexmihailov changed the title Support encode/decode java.time.Duration and ConfigMemorySize, decode java objects in Java Bean notation in HOCON Support encode/decode java.time.Duration and ConfigMemorySize in HOCON Feb 17, 2023
Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

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

OK, only file name and minor comments left (please re-check Github's hidden converstaions section)


/**
* Serializer for [ConfigMemorySize].
* For decode using [HOCON Size in bytes format](https://github.com/lightbend/config/blob/main/HOCON.md#size-in-bytes-format).
Copy link
Member

Choose a reason for hiding this comment

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

Bump: Rewrite as 'All possible Hocon size formats....'


@Test
fun testSerializeDuration() {
Hocon.encodeToConfig(Simple(ofMinutes(10))).assertContains("d = 10 m")
Copy link
Member

Choose a reason for hiding this comment

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

Same here: why don't write function similar to testMemorySize?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented the testJavaDuration function

@sandwwraith
Copy link
Member

@alexmihailov Hi, do you have time to finish this on this week? I plan to release 1.5.0 soon.

@alexmihailov
Copy link
Contributor Author

Hello! Thursday/Friday I will try to finish

@alexmihailov
Copy link
Contributor Author

@sandwwraith Hi! I checked the hidden sections of conversations and made corrections. I'm sorry I didn't notice right away.

Copy link
Member

@sandwwraith sandwwraith left a comment

Choose a reason for hiding this comment

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

Thank you very much for all your time!

@sandwwraith sandwwraith merged commit acb0988 into Kotlin:dev Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants