Commit a58e621
[bindgen][kotlin] Enforce passing required fields to the record builder constructor. (#7330)
Fix https://mapbox.atlassian.net/browse/CORESDK-3976
## Problem
We faced some big integration problems with BMW and them using our
Kotlin generated Bindgen classes directly. The problem is that they are
constructing objects using builder pattern directly (e.g. this
[build()](https://github.com/mapbox/mapbox-sdk/blob/d4d47465dc68badddf75672b58a91c90691f9676/projects/navigation-sdk-cpp/modules/hd/generated/public/kotlin/com/mapbox/navsdk/hd/LaneChangeAssistData.kt#L95-L105))
and whenever they forget to pass some required property in the builder -
we get the runtime crash instead of the compile time error (which is
especially nasty when it happens during actual drive in some specific
situation). My question is - why don’t we require to pass mandatory
arguments to constructor of the builder?
For context, previously for records, we generate a builder with empty
constructor, and user can build the record with empty constructor and
set the parameters as follows:
```
// for example X is required non-null field and Y is optional/nullable field
val record = Record.Builder().setX(0).setY(1).build()
val buildRecordWithDsl = record {
x = 0
y = 1
}
// following will result in runtime exception
val record1 = Record.Builder().setY(y).build()
val buildRecord1WithDsl = record {
y = 1
}
```
And we will throw runtime exception if some required non-null fields are
not set by the user.
## Proposed changes
As
[discussed](https://mapbox.slack.com/archives/CHL04L955/p1760019630150189)
with @jush @kiryldz and @yunikkk we decide to proceed with breaking the
existing API surface, e.g. remove the empty builder constructor and
require user to always pass in required fields to the builder
constructor for following reasons:
* Simplicity - it would be much simpler to keep only one constructor and
make it clear what is needed to avoid misuse.
* Runtime safety - by removing empty builder constructor, we make sure
that user wouldn't encounter runtime crashes while building a record
without specifying the required fields.
* To avoid confusions - as @jush discovered the `MapboxDelicateApi`
annotation wouldn't work on Java, so exposing two builder constructors
would result in bad Java developer experience.
* And we validated that it's okay to break this API at this point for
all the affected projects(for nav-sdk-cpp the generated code is
internal, and for geogencing APIs it's still experimental). We still
have the option to introduce empty constructor later if there's customer
complaints, as it would then be incremental change.
There are still a few considerations:
* API surface diverge from Java generator
In Java generator, we only have empty builder constructor, and we crash
at runtime when required field is not provided, this behaviour is
different than Kotlin generator, however as discussed with the team, we
feel making sure a safe and developer friendly API is more important
than enforcing exact alignment with legacy generator, and in JNI we use
private record constructors anyway so it shouldn't cause problems to our
JNI layer.
* API breaking change could happen if new required fields are added to
the record.
In this senario, it would be a runtime exception before this change and
in Java generator, it would also cause significant customer impact, so
we are in similar situation.
Additionally I added a bindgen warning log when a required field is add
to a record, so we will be aware and make intentional decision.
## Changes in this PR
* Update the record builder constructor to enforce passing all the
required fields.
```
// for example X is required non-null field and Y is optional/nullable field
val record = Record.Builder(x = 0).setY(1).build()
```
* Update the DSL builder function to construct the record with required
arguments.
```
// for example X is required non-null field and Y is optional/nullable field
val buildRecordWithDsl = record(x=0) {
y = 1
}
```
* Make DSL builder's init block default to empty, as all the required
fields area already enforced to be passed, user can simply skip the init
block by default.
```
// for example X is required non-null field and Y is optional/nullable field
val buildRecordWithDsl = record(x=0)
```
* Bindgen will log warning messages when required field is added to a
record, below is an example of such message:
```
Detected required field 'c' for record 'RecordWithOptional', please double check this is intended, consider making it optional or provide default value to avoid breaking changes.
```
cc @mapbox/core-sdk
cc @mapbox/nav-core-sdk
cc @mapbox/3d-live-navigation
cc @mapbox/maps-android
cc @mapbox/sdk-ci
---------
Co-authored-by: Changelog autocreator bot <[email protected]>
GitOrigin-RevId: 894873471d9d5ed940103c890ab6c618c0edf0021 parent eeb0e2d commit a58e621
File tree
2 files changed
+6
-3
lines changed- app/src/main/java/com/mapbox/maps/testapp/examples/geofence
2 files changed
+6
-3
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
8 | 11 | | |
9 | 12 | | |
10 | 13 | | |
| |||
Lines changed: 3 additions & 3 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
427 | 427 | | |
428 | 428 | | |
429 | 429 | | |
430 | | - | |
431 | | - | |
| 430 | + | |
| 431 | + | |
432 | 432 | | |
433 | | - | |
| 433 | + | |
434 | 434 | | |
435 | 435 | | |
436 | 436 | | |
| |||
0 commit comments