DokkaConfiguration: Use LinkedHashSet instead of Set to preserve order for includes when deserialize (#2999)

[eunwoop]

• for markdown files for multi module description

[IgnatBeresnev]

Hi! Does it resolve #2999? It'll be surprised if it does, because I believe .toSet() already creates LinkedHashSet internally, so there should be no change in behavior between your proposal and the current implementation.

[eunwoop]

It’s because DokkaConfigurationImpl is serialized and then deserialized using JSON and the order is not preserved during that process.

When I print the log before serialization, the order of elements in the includes is preserved. However, after deserializing the JSON string and using a Set type for the includes field, the order is not preserved.
I guess Jackson does not guarantee the order of fields for Set types.
So when I changed the includes field's type to LinkedHashSet, it preserves the order of elements during deserialization.

The serialization and then deserialization happens here -->

In AbstractDokkaTask, the buildDokkaConfiguration() method returns a DokkaConfiguration object, and then toCompactJsonString() method is called to convert it into a JSON string.

    @TaskAction
    internal open fun generateDocumentation() {
        DokkaBootstrap(runtime, DokkaBootstrapImpl::class).apply {
            configure(buildDokkaConfiguration().toCompactJsonString(), createProxyLogger())

And then inside DokkaBootstrapImpl.configure(), the DokkaConfigurationImpl is created with the serialized JSON configuration.

    override fun configure(serializedConfigurationJSON: String, logger: BiConsumer<String, String>) = configure(
        DokkaProxyLogger(logger),
        DokkaConfigurationImpl(serializedConfigurationJSON) // HERE
    )

[IgnatBeresnev]

However, after deserializing the JSON string

Oh yeah, I completely forgot about it.. what a strange bug... Thanks for diving deep and researching it!

Using LinkedHashSet and breaking the Set abstraction kind of feels bad, and it'll be a breaking API change for some of the clients (since this is public API)..

Please, give me a few days to discuss this with my colleagues, maybe we can think of something. I'll try to get back to you on Friday

[eunwoop]

@IgnatBeresnev
Hello, I updated my change to avoid breaking API change.
Instead, I used addAbstractTypeMapping for objectMapper.

[eunwoo-park-nhn]

@IgnatBeresnev Hello I updated the PR. Could you reveiw this?

[IgnatBeresnev]

Hi! Yes, sorry for the delay and thank you for not giving up :)

[IgnatBeresnev]

Neat solution, and I think it makes sense!

If you have the time, could you please add a simple test to core/src/test/kotlin/utilities/JsonKtTest.kt for the parseJson function? Just to make sure it doesn't break in the future

[IgnatBeresnev]

Thanks!