Builders vs. Data Classes

[cprice404]

Hi, thanks for all of your work so far on the Kotlin SDK! We're very excited that it exists and counting the days until it moves out of alpha/beta into GA :)

First thing to note is that the killer feature for us is the suspend functions, by a mile. We're being somewhat aggressive about trying it out / adopting it just because of how big of a difference that makes in our kotlin code base.

One thing I have been noticing though, that I wanted to inquire about, is how the builders work.

For example, for the DDB client, to create a table, I have to do something like this:

        val request = CreateTableRequest {
            attributeDefinitions = listOf(attDef)
            keySchema = listOf(keySchemaVal)
            provisionedThroughput = provisionedVal
            tableName = newTableName
        }

many (most? all?) of those fields are required, but because of the way this DSL works, I can delete e.g. provisionedThroughput and this code will still happily compile, and I won't know I have a problem until runtime.

One of the main value propositions of Kotlin for us is that it is so diligent about pushing errors forward from runtime to compile time. If I was designing a DSL for these builders from scratch, my inclination would be to use data classes rather than this builder block DSL.

With a data class, the syntax would look almost the same:

        val request = CreateTableRequest(
            attributeDefinitions = listOf(attDef)
            keySchema = listOf(keySchemaVal)
            provisionedThroughput = provisionedVal
            tableName = newTableName
        )

but if I removed a field, e.g. provisionedThroughput, then one of two things would happen. Either your data class provides a default value for that field and I'm fine, or it doesn't, and I get a compile-time error instead of a runtime error.

Without any familiarity with how your codegen works, I don't know whether there is some technical reason why it's easier to generate this builder block DSL than it would be to generate data classes, or if you have some other motivation for this design choice. But I wanted to ask about it because it feels like it strays from kotlin idioms a bit and I'm curious to know if it would be possible to flip to data classes.

[aajtodd]

Hey Chris, thanks for taking the time to provide this feedback. This is such a great question and one that comes up often so I'm going to try and explain how we landed here and where we may be going.

First thing to note is that the killer feature for us is the suspend functions, by a mile. We're being somewhat aggressive about trying it out / adopting it just because of how big of a difference that makes in our kotlin code base.

This is great to hear!

Without any familiarity with how your codegen works, I don't know whether there is some technical reason why it's easier to generate this builder block DSL than it would be to generate data classes, or if you have some other motivation for this design choice. But I wanted to ask about it because it feels like it strays from kotlin idioms a bit and I'm curious to know if it would be possible to flip to data classes.

It has less to do with easier to generate and more that there are technical limitations in the way services are modeled (more or on that in a second). There are two things here that you are getting at really (1) the use of data classes vs builders and (2) better nullability information in generated types.

Data classes vs Builders

There are a few things that influence this decision.

1. Models aren't static and evolve over time
2. Complex construction is better supported with a DSL

Model Evolution

(1) is the most important so let's start with that. Let's take a hypothetical Smithy model (see appendix for more information):

service FooService {
    version: "2006-03-01",
    operations: [CreateFoo]
}

operation CreateFoo {
    input: CreateFooRequest,
    output: CreateFooResponse
}

structure CreateFooRequest { 
    @required
    string name
    
    string description
}

structure CreateFooResponse { ...  }

The required trait is what would tell us that we could theoretically generate a type as T instead of T? on the input. If we were to generate a hypothetical data class for the operation input it might look like:

data class CreateFooRequest(
    name: String,
    description: String? = null
)

The issue is that it is actually a non-breaking change to remove the @required trait from a member. If we release a version of the SDK with this type and a service evolves in such a way that member is no longer required, i.e it looks like this now.:

structure CreateFooRequest { 
    string name,
    string description
}

Then suddenly we would output:

data class CreateFooRequest(
    name: String? = null,
    description: String? = null
)

This is breaking change in the generated SDK (whether we use data classes or builders). Of course we could major version bump the SDK but this evolution happens enough that it would be a poor customer experience as well.

The other issue with model evolution is the addition of members. If an (optional) member is added to a structure:

structure CreateFooRequest {
    @required
    string name,
    integer priority,
    string description
}

For this to not be a breaking change we have to guarantee the order that members are generated in because consumers may be using positional arguments (there is no way to force named arguments).

e.g. if we mistakenly generate priority in the middle of existing arguments it would be a breaking change:

// after model update that added `priority`

data class CreateFooRequest(
     name: String,
     priority: Int? = null,
     description: String? = null
)

// version 1
val request = CreateFooRequest("name", "description")

// error, priority became the second constructor argument which is of type Int
val request = CreateFooRequest("name", "description")

Not only would this break the constructor but the copy() and componentN() functions as well.

This issue is actually solvable but it requires that members always be added to the end of a structure. This is supposed to be the way models evolve but nothing actually guarantees this. It was deemed safer to use a DSL builder to prevent accidental backwards compatibility issues than rely on something that isn't guaranteed.

Complex Inputs

From less of a technical constraint we chose DSL syntax over data classes because it supports more complicated inputs. For example any operational input that has a nested structure type benefits from a DSL. Let's take a DynamoDB example:

val req = CreateTableRequest {
    tableName = "table-name"

    ...

    // this is a nested class/structure type, the DSL allows you to create it using the ProvisionedThroughput.DslBuilder
    provisionedThroughput {
        readCapacityUnits = 10
        writeCapacityUnits = 10
    }
}

Additionally there are some operations that have a very large number of inputs (e.g. S3 PutObjectRequest has 32 possible input fields). The construction of these using a data class starts to become cumbersome and subjectively harder to read.

Finally, DSL builders allow us to generate overloads for the service client interface that allow you to not create a request in most cases:

val createTableResp = ddbClient.createTable {
    tableName = "table-name"
    
    ...

    provisionedThroughput {
        readCapacityUnits = 10
        writeCapacityUnits = 10
    }
}

Nullability of Generated Types

The model evolution section above touched on this but the main issue today is that there is nothing in the model that really allows us to generate required members as non-nullable. All three new AWS SDKs Kotlin, Rust, and Swift all model optionality/nullability directly in their type system and all share this concern. We recognize that this is not ideal and there is some work going on to try and improve the status quo but nothing concrete to share at the moment.

Hopefully that gives you some context around both the technical constraints as well as some of the subjective decisions made. Please feel free to ask any additional questions and of course we would love to hear any feedback you have about using the SDK (good or bad).

Appendix

• Smithy
  • IDL language that AWS uses for service models and code generation
• Smithy Kotlin Design Docs
  • Data Classes vs Builders

[cprice404]

Thanks @aajtodd for the very thoughtful and detailed response! The issues you raise re: backward compatibility make sense. It's unfortunate because it really does make the API feel a little un-kotlin-like to me, but I understand the reasoning now. I suspect our team will probably end up writing a shim layer that uses data classes and sits in front of the SDK APIs that we need to use most frequently.

[kvcache]

Hi @aajtodd, a few thoughts sprung to mind as I read your reply:

On Data classes vs Builders

Claim 1: Models aren't static and evolve over time

While this is fundamentally a truth, there is an obvious utility to constraining one's model to some concept of temporal consistency. For example, what features could DynamoDB roll out which would make this snippet make sense?

val createTableResp = ddbClient.createTable { }

Full disclosure: I was an engineer who worked on the DynamoDB control plane component that runs the Create Table job, as well as on the low-level services on which those tables store their data.

I cannot (so far) come up with any sensible way for this to be a valid invocation of createTable and I’ll use it as an example of an invalid/nonsense use of a ddbClient.

If we agree that ddbClient.createTable { } is nonsense today and forevermore, then why should we defend the expression as a valid, compilable construct?

Fundamentally, DynamoDB tables are named. Changing that would surely be a major version bump! Kotlin tries to push bugs toward the compiler, toward the developer, helping developers to resist bugs; not unlike Rust with its rigorous guardrails around data ownership and concurrency semantics.

Service models aren't static and they change over time. That’s surely true, I agree. data classes can also change over time and they truthfully, helpfully surface conflicting changes to developers on upgrade rather than on deployment at runtime in front of their users if their test coverage isn't perfect.

Here are a few helpful things to consider:

• How should a user of the Kotlin DSL go about upgrading their AWS Kotlin SDK dependency?
• Which users will you support? Will you support users who don’t have 100% test coverage?
• How much test coverage must a developer have in their code before the AWS Kotlin SDK is safe for them to upgrade?
• Does the AWS Kotlin SDK make it easy to produce ill-formed programs?
• Does the AWS Kotlin SDK make it easy to produce well-formed programs?
• Compilers touch nearly 100% of the code in a typical JVM application. Is it clear that expressing well-formedness at compile time is a form of test coverage?
• How hard would it be for a user to ship an upgrade bug to a production environment with the DSL?
• How hard would it be for a user to ship that bug to a production environment with data classes?

Then suddenly we would output [CreateFooRequest data class with optional name field]

I'm with you so far this makes sense.

This is breaking change in the generated SDK (whether we use data classes or builders). Of course we could major version bump the SDK but this evolution happens enough that it would be a poor customer experience as well.

So if I accept that this is a breaking change (which in the case of a request object I do not but I will go with it for the sake of this discussion), then I think you’re suggesting that breaking changes should be advertised as minor version bumps? With data classes you’d get a compile time build break which is okay, but what about users who take a minor version and just ship it assuming there are mundane security fixes and whatnot?

Will those users discover the breaking change snuck into a minor version at build time (early) or at test time (hopefully at least here) or at deployment time? With the DSL can you control that? Could you control when the user is confronted with an ill-formed program with data classes?

Consider an API-breaking change you might want to release as a minor version bump (for whatever reason, I don’t have an example, this isn’t something I generally condone but just imagine some change here that adds a required parameter or something that you’d release with a minor version bump under your rubric)

Let’s take it as a given that you’d like to release in a way that adheres to Tenet #1 of the AWS SDK and Tools Team which was helpfully linked above, and promotes “Developers can depend on our code.” I will interpret that in this context to apply not only to 1 jar, but transitively to upgrades as well; stated plainly, “developers can depend on upgrades” which consist of AWS sdk code. As a user of the SDK and a developer, personally my trust for any SDK is rooted in and limited by how likely it is to cause my users pain.

• If you have a DSL, how can users of the SDK discover if they are impacted by the change even if they are diligently reading your change notes and see the notice of that change?
• If you used data classes instead, do you even need you user to read your change notes?

if we mistakenly generate priority in the middle of existing arguments it would be a breaking change

I freely agree with this! But is this a problem for your users or is this a problem for your team? Isn’t a mis-generated SDK a bug? By letting a bugged SDK go out, and blessing it as a real release, which strategy handles this situation better from a user perspective?

• Does the DSL allow arbitrary mis-generations of sdk flow to users’ production runtime JVMs?
• Do data classes?

This issue is actually solvable but it requires that members always be added to the end of a structure. This is supposed to be the way models evolve but nothing actually guarantees this. It was deemed safer to use a DSL builder to prevent accidental backwards compatibility issues than rely on something that isn't guaranteed.

It sounds to me like you might believe that data classes are safer and better if there were an ordering guarantee from the generator? Have you opened an issue against the relevant team? Where can I go to 👍 it?

Claim 2: Complex construction is better supported with a DSL

I totally disagree with you! This is subjective and significant usability implications hinge on its outcome. Let’s take a look:

val req = CreateTableRequest {
    tableName = "table-name"
    provisionedThroughput {
        readCapacityUnits = 10
        writeCapacityUnits = 10
    }
}

Versus

val req = CreateTableRequest(
    tableName = "table-name"
    provisionedThroughput = ProvisionedThroughput(
        readCapacityUnits = 10
        writeCapacityUnits = 10
    )
)

If DynamoDB adds a requirement that a CreateTableRequest has a tableNamespace parameter or a ProvisionedThroughput adds a requirement that you plug in a maximumBurst argument:

• Can your users upgrade with confidence per Tenet #1 with a DSL? (Runtime exception)
• Can they upgrade with confidence per Tenet #1 with data classes? (Build break)

Which of those code snippets is easier to explain to a developer who is new to this SDK? Which of those code snippets helps developers provide at least the correct required parameters?

I don’t deny that the DSL is neat, but until Kotlin contracts can verify that block parameters invoked something else (i.e., required parameters) AT_LEAST_ONCE it looks to me like all of the first-hour experiences with all apis and every upgrade will imply having to re-read all the documentation every time to see what is currently required to avoid crashing at runtime. :sad:

Additionally there are some operations that have a very large number of inputs (e.g. S3 PutObjectRequest has 32 possible input fields). The construction of these using a data class starts to become cumbersome and subjectively harder to read.

Please look again at the code sample I produced above. There are extra characters required only for nested object types. Otherwise, users just name the field and assign the value with data classes, right? Isn’t that the same thing as a basic DSL? Isn’t a data class constructor little more than a tiny localized Data DSL? If you're assigning 32 values into a data class or 32 items in a DSL you're doing something pretty involved! When S3 adds a 33rd field which happens to be required, wouldn't it be nice for the poor developer who has to fix that giant expression to have the compiler tell them about the new field and that it's missing instead of, e.g., noticing that their database backup job they forgot to write an alarm for hasn't been working for 3 months because nothing told them they needed to plug a new required parameter when they updated their Kotlin AWS client?

On Nullability of Generated Types

Wow, this probably should have gone up at the top haha 😅. If you are technically incapable of generating optional fields due to some upstream limitations then your hand is forced and your users are constrained to handle these mundane breaking SDK upgrade bugs themselves, at runtime, by reading change notes and grepping through their code for usages that need a new or renamed parameter passed. I don’t think Kotlin provides any tractable way around this without addressing that core upstream issue. If it blocks users from the right SDK experience how high of a priority is fixing this issue in the relevant upstream?

Epilogue

We recognize that this is not ideal and there is some work going on to try and improve the status quo but nothing concrete to share at the moment.

With respect, I think this is an extreme understatement and I’ll plainly state my opinion: This SDK, as it stands, promotes unsafe upgrades culturally (an expressed reluctance to bump major versions on changes that are considered to be "breaking") as well as technically (hiding required parameters behind complex dynamic types instead of simple compile time constructs).

I'd like to bring it back to Tenet #1: Developers can depend on our code

I fervently want to. In the time-honored tradition of developers everywhere, I implicitly depend on my compiler. Code whose correctness is rooted in my compiler is code I am readier to believe I can depend on, at least as far as I can depend on my compiler - which is pretty far these days, compilers are quite good!

[aajtodd]

@kvcache Thanks for the detailed feedback.

This SDK, as it stands, promotes unsafe upgrades culturally (an expressed reluctance to bump major versions on changes that are considered to be "breaking") as well as technically (hiding required parameters behind complex dynamic types instead of simple compile time constructs).

I want to address this last bit of feedback first because I think it's important that users of the AWS SDK for Kotlin understand that upgrades are safe.

Many of the examples you have given are based on an invalid model evolution. Addition of the @required trait is generally considered a breaking change and will be blocked from release. There have been some instances in the past of service teams allowed to add it after a model has shipped without it but only if it was already being enforced server side (i.e. a customer would have always had to supply a value for the request to be valid in the first place). The Kotlin team has taken a lot of care to ensure that all currently allowed service model changes won't result in a breaking change for our users at either compile time or runtime.

This has some tradeoffs of course, some you and many others may disagree with. But we as a team feel strongly that backwards compatibility is of the upmost concern. Users must be able to upgrade their SDK to take advantage of new API's, bug fixes, performance updates, or even security fixes without being forced to change their existing code. We never want to break our current customers that already built something using our services.

OK with that out of the way, I'll try to respond to some of your other feedback.

A lot of your feedback seems to be centered around a desire to have the compiler tell you that a request is valid (i.e. not missing a required parameter). As you noted, with a DSL builder the compiler can’t tell you this. This is the current status quo of all AWS SDKs and is a property of allowed model changes (however unlikely).

While this may not be seen as ideal, I do think it is less of an issue than one might think. Software written by AWS customers isn't written and shipped just because the compiler says it's good to go. Users are trying to do something: create a table, put data into a bucket, etc. They are presumably going to be testing their code works. Any issues w.r.t required parameters should be trivially found during development. I can't deny that the compiler would be a faster forcing function, but I am confident users will get there very fast even if they do hit a missing required parameter issue. Once a customer has a valid request, future SDK updates will not cause a breaking change.

if we mistakenly generate priority in the middle of existing arguments it would be a breaking change

I freely agree with this! But is this a problem for your users or is this a problem for your team? Isn’t a mis-generated SDK a bug?

My wording here could have been better. The service teams are responsible for their models and nothing today blocks a team from shipping a model with a member added in the middle of existing members. We do ask teams to append new members to the end but this is a relatively new requirement. Many service teams have built tooling for their models over the years that does not and cannot easily adhere to this. Our choice to not pursue a constructor with default arguments is based on providing strong backwards compatibility guarantees. If a member were to be added in the middle the best case scenario is that it would be caught by the compiler due to type differences. If the types happen to be the same this could go unnoticed for quite some time.