Consider adding API for changing deeply nested values

[pindab0ter]

The documentation currently doesn't provide an explanation for how to change deeply nested values. The official documentation on generated Java code provides an explanation of exactly this case.

[andrewparmet]

In the case of Java Protobuf, they've built and are showcasing a special feature of the builder API to reduce the amount of code needed to modify deeply nested fields. We don't have anything purpose-built, but the DSL provides a reasonably clean way to do this sort of thing out of the box.

Using their example:

message Foo {
  int32 val = 1;
  // some other fields.
}

message Bar {
  Foo foo = 1;
  // some other fields.
}

message Baz {
  Bar bar = 1;
  // some other fields.
}

We can modify deeply nested fields:

baz = baz.copy {
    bar = bar!!.copy { // `bar` references the DSL's value of `baz.bar` which is set when invoking `copy()`
        foo = foo!!.copy { // same here for `foo` and `baz.bar.foo`
            `val` = 10
        }
    }
}

It might be counterintuitive that you can reference bar and foo without having to refer to baz.bar and baz.bar.foo. This could be worth adding to the readme.

[pindab0ter]

Is there any way for the DSL to allow this at some point?

baz = baz.copy { bar.foo.`val` = 10 }

[andrewparmet]

When you're in the context of baz.copy { }, bar.foo is an instance of Foo, not a FooDsl. Instances of Foo are immutable, so it would not be a straightforward change. Somehow bar.foo would have to become a reference to FooDsl while still allowing assignment of instances of Foo to bar.foo.

If we wanted to copy the Java library, it might look like exposing bar.foo as a function, perhaps:

baz = baz.copy { bar!!.foo { `val` = 10 } }

[pindab0ter]

So I'm correct in understanding that a chained call will not be possible:

baz = baz { bar.foo.`val` = 10}

From a syntactic sugar point of view it would be much preferable to be able to pass a lambda to a (nullable?) instance and have it implicitly be a call to .copy { }. In our code examples I think we both forgot the assignment, but I think it would still be very nice to not have to specify the .copy { } call every time.

baz = baz { bar = bar { foo = foo { `val` = 10 } } }

[andrewparmet]

I did some experimentation and it is possible to define a method that behaves the way you propose:

class Bar(...) : KtMessage {
    class Deserializer : KtDeserializer<...> {
        operator fun Bar?.invoke(dsl: BarDsl.() -> Unit) =
            Bar {
                if (this@invoke != null) {
                    foo = this@invoke.foo
                    unknown = this@invoke.unknown
                }
                dsl()
            }
        }
    }
}

I have a few concerns with this approach though: we chose copy as a name for this operation to remain consistent with the "flavor" of data classes, since there is a close resemblance between protobuf objects and Kotlin data classes. We want KtMessages to behave like data classes - and they would be data classes if we could guarantee that reordering fields in a proto file wouldn't break function calls, but we can't. We could choose to move away from copy, but it's a decision we'd have to consider carefully.

Also, a nullable receiver (by design) sweeps the inherent protobuf nullability of the property under the rug. Part of the advantage of using protokt over protobuf-java is that the optional nature of fields with message types is exposed as nullability of their associated Kotlin properties, which forces users to carefully consider the possible states of a deserialized object. I could see a situation where you commit a capital-letter typo leading to inadvertently copying an instance instead of creating a new one:

In our nullable-receiver world, consider a situation where bar is usually null. These two calls do the same thing:

baz = baz {
    bar = bar {
        foo = foo {
            `val` = 10
        }
    }
}

baz = baz {
    bar = Bar {
        foo = foo {
            `val` = 10
        }
    }
}

Suppose you always want to create a new instance. One day if you get an instance of Baz with a non-null bar, your code will start copying any other fields that have come along the wire. It would be easy to miss the source of your bug - but if you'd been forced to use the method copy, it would be obvious what was happening. The behavior you wanted required a capital letter!

If the receiver was not nullable, you'd be forced to use !!, at which point you would be asking the important nullability questions and programmatically handling the two cases anyways, making this extra method worth little in terms of terseness. If you knew you always wanted bar to exist and you're willing to live with the consequences if it is null, you could use the (protokt.property).non_null option and you won't have to use !!. Then .copy isn't so ugly in my opinion, because its usage looks a lot like a data class.

This is of course getting away from the original issue about modifying deeply nested fields more concisely. Google's toBuilder() recursively converts all nested message types to builders so you can go in and mutate nested builders. We don't do that - the builder is only created on command when you .copy a KtMessage. But I think Kotlin's inherent terseness relative to Java lends well to this operation without losing too much by having to do it the long way.

[pindab0ter]

Thank you for your detailed explanation. I'm still quite new to protobuf in general so I appreciate you explaining the reasons behind certain decisions.

I'd love a marriage of what philosophy you're currently working with and the way Java uses their builder, but I understand that it's either/or, or you risk bloat since there's no big reason to do so.

[andrewparmet]

A small correction to my post above: it looks like protobuf-java doesn't eagerly automatically convert nested messages to builders, but instead allows programmatic setting of messages as if they were builders, doing conversion on the fly and invalidating previously set versions of the nested message as a message.

Short of alternative naming conventions I think the unification would be to expose the lowercase builder. I'll have to keep thinking about it and bring it up internally if there's a strong desire to support that pattern. Do you mind if I close this issue for now?

[pindab0ter]

I do hope you will end up finding a way to support a builder-like pattern that enables easy manipulation of (deeply) nested messages. It is common for me to use a message inside a list inside a message inside yet another message. Currently a lot of what feels like boilerplate is involved. I look forward to hearing more, but feel free to close this for now.

[andrewparmet]

I'll leave it open with a name better reflecting the eventual goal.