Avoid nested constructor data binding if there are no request parameters

[flpa]

Note: I also raised this topic as a SO question: https://stackoverflow.com/q/77645476/827950

When attempting to upgrade our Spring Boot + Spring Web based REST API from Spring Boot 3.1.6 to 3.2.0, we encountered an unexpected change of behavior in databinding of query parameters. In some cases it leads to errors, in others it might cause tricky bugs.

The following example shows our situation: a REST controller with a simple GET endpoint that retrieves its parameters as an object that contains a nested object:

@RestController
@RequestMapping("/builder")
public class DemoLombokDataBuilder {

   @Data
   @Builder
   static class Dto {
      String field;
      Nested nested;
   }

   @Data
   static class Nested {
      String nestedField;
   }

   @GetMapping
   public String demo(Dto dto) {
      String result = dto.getField();

      if (dto.getNested() != null)
         result += dto.getNested().getNestedField();
      else
         result += ", nested is empty";

      return result;
   }

With Spring Boot 3.1.6, the result of calling the endpoint with or without the nested field are:

URL	Result
http://localhost:8080/builder?field=foo	foo, nested is empty
http://localhost:8080/builder?field=foo&nested.nestedField=bar	foobar

With Spring Boot 3.2.0 however, the results are:

URL	Result
http://localhost:8080/builder?field=foo	foonull
http://localhost:8080/builder?field=foo&nested.nestedField=bar	foobar

So instead of not instantiating the nested object at all when its field is not provided, Spring Boot 3.2 instantiates the nested object with empty values. This can lead to bugs - and it's even worse when validation is used and the nested object has mandatory fields annotated with @NotNull. In that case Spring Boot 3.2 throws HTTP 400, while the requests worked fine in 3.1.

To us, this seems like a change between minor version that can cause existing code to break, and therefore a bug. Is this expected behavior or did we miss something from the docs or upgrade guides?

Additional Details

In my further tests, the issue only occurs when the DTO class can only be constructed via an all-args-constructor.
With this DTO:

   @Data
   static class Dto {

      String field;
      Nested nested;
   }

both 3.1 and 3.2 have the same results:

URL	Result
http://localhost:8080/builder?field=foo	foo, nested is empty
http://localhost:8080/builder?field=foo&nested.nestedField=bar	foobar

I pushed the full examples to this Github repo: https://github.com/flpa/springboot-databinder-change-tests

[sbrannen]

Hi @flpa,

Congratulations on raising your first issue for the Spring Framework. 👍

I see that your example uses Lombok which prevents us from analyzing it without special tooling, which does not exist on GitHub and which we do not wish to install in our IDEs.

Can you please update your example so that it reproduces the issue without Lombok involved?

Thanks

[flpa]

Hey @sbrannen,
Thanks!

Sure, I've added controllers based on plain Java in the repo, they look like this:

@RestController
@RequestMapping("/plain-java")
public class DemoPlainJava {

   static class Dto {
      private String field;
      private Nested nested;

      public Dto(String field, Nested nested) {
         this.field = field;
         this.nested = nested;
      }

      public String getField() {
         return field;
      }

      public void setField(String field) {
         this.field = field;
      }

      public Nested getNested() {
         return nested;
      }

      public void setNested(Nested nested) {
         this.nested = nested;
      }
   }


   static class Nested {
      private String nestedField;

      public String getNestedField() {
         return nestedField;
      }

      public void setNestedField(String nestedField) {
         this.nestedField = nestedField;
      }
   }


   @GetMapping
   public String demo(Dto dto) {
      String result = dto.getField();

      if (dto.getNested() != null)
         result += dto.getNested().getNestedField();
      else
         result += ", nested is empty";

      return result;
   }

The behavior is the same.

[felixscheinost]

I also just stumbled across this when migrating a Kotlin Spring Boot application from 2.7 to 3.2.

EDIT: THIS EXAMPLE IS WRONG, this also didn't work in 2.7, see reply below

@Controller
class TestController {
  @GetMapping("/")
  fun test(form: Form): String {
    return "test"
  }
}

class Form(
  // When `someOptionalObject` is missing I expect the databinder to call the constructor with null
  // 2.7.x did this
  // 3.2.0 tries to construct a `SomeOptionalObject` and fails.
  val someOptionalObject: SomeOptionalObject? = null
)

class SomeOptionalObject(
  val someString: String
)

There we have a @ModelAttribute which has an optional attribute which is a nested class, like here.

I checked the source and previously 2.7.x read the JSR-305 annotation and passed null to the constructor.

2.7.x did the correct thing here:

spring-framework/spring-web/src/main/java/org/springframework/web/method/annotation/ModelAttributeMethodProcessor.java

Lines 288 to 290 in d54e101

	if (value == null && methodParam.isOptional()) {
	args[i] = (methodParam.getParameterType() == Optional.class ? Optional.empty() : null);
	}

3.2.x recursively tries to call DataBinder for SomeOptionalObject which fails. It doesn't consider null a possible value for the constructor:

spring-framework/spring-context/src/main/java/org/springframework/validation/DataBinder.java

Line 951 in c75c0ae

if (value == null && shouldConstructArgument(param)) {

As a workaround I tried adding a no-arg constructor using @JvmOverloads but DataBinder refuses to use that (KotlinDelegate.findPrimaryConstructor always returns the primary one)

[rstoyanchev]

@flpa thatnks for the report and sample. It sounds like the constructor exposes two arguments, but when used with data binding, only one is ever expected to be populated, is that right? Appears the same your case @felixscheinost, and likely in #31800 as well. Just trying to confirm my understanding.

[felixscheinost]

Yeah this sounds like a duplicate of #31800 to me as well.

In my case the actual Form has lots of parameters but all of them are nullable (for GET) and some combination of them are populated in various other cases (POST).

The fix in #31800 looks weird to me. Isn't value == null always the case, even if e.g. in my case someOptionalObject.someString was set? Because resolveValue("someOptionalObject", ...) wouldn't find a value for "someOptionalObject"?

I would expect the logic to be like: "if the value is null, try databinding a object recursively using createObject, if that fails due to no parameters being set AND if the parameter is optional, then use null"

EDIT: I see you also noted that the fix is likely incorrect, sorry.

[rstoyanchev]

In my case the actual Form has lots of parameters but all of them are nullable (for GET) and some combination of them are populated in various other cases (POST).

That's the question though. How were they getting populated for POST since before Spring Framework 6.1 (Boot 3.2) we did not support nested constructor binding?

[felixscheinost]

Sorry, you are right I mixed something up here!

The mixup was between two different variants of this pattern that are both used in the application I am migrating.

The example I posted above, with the constructor containing val also wouldn't have worked in 2.7.x, like you said.

First variant which works in both 2.7 and 3.2 (val with default + nullable var)

class Form(
  // POST will bind `someOptionalObject.someString`
  val someNestedObject: SomeNestedObject = SomeNestedObject()
)

class SomeNestedObject(
  var someString: String? = null,
)

In this case we would include a parameter someOptionalObject.someString in the request.

Second variant using JSON + custom converter which broke during migration

This variant is like I described above BUT it used a custom converter.

class Form(
  // POST will include `someOptionalObject = "<JSON>"` which is converted by custom Converter
  val someOptionalObject: SomeNestedObject? = null
)

class SomeNestedObject(
  val someString: String,
)

class SomeConverter : ConditionalGenericConverter {
  override fun getConvertibleTypes() = setOf(
    GenericConverter.ConvertiblePair(String::class.java, SomeNestedObject::class.java),
    GenericConverter.ConvertiblePair(SomeNestedObject::class.java, String::class.java)
  )

   // ...
}

I have a runnable example here: https://github.com/felixscheinost/databinder-form-nullable

In the build.gradle.kts file, the 4 lines in the plugins block can be commented/uncommented to switch between 2.7 and 3.2

In 2.7:

$ curl http://localhost:8080
someString=null

$ curl "http://localhost:8080?someOptionalObject=JSON"
someString=JSON

In 3.2:

$ curl http://localhost:8080
{"timestamp":"2023-12-12T21:05:04.258+00:00","status":400,"error":"Bad Request","path":"/"}                                                                                                                                                                                                             

$  curl "http://localhost:8080?someOptionalObject=JSON"
someString=JSON

We use JSON here to pass around a single object with internal structure as a single parameter. The object is static and using JSON compared to e.g. multiple request parameters and nullable vars has the benefit that we can't bind half an object: It is either all or nothing, which we want in this case.

In the example project the converter is simplified but the principle is the same.

[flpa]

@rstoyanchev yes you are right, seems like this is the same topic as in #31800.
You noted:

It sounds like the constructor exposes two arguments, but when used with data binding, only one is ever expected to be populated, is that right?

I'd expect the two arguments to be populated depending on the invocation of the endpoint. In my example, when the URL contains a value for "field", the constructor argument "field" should be populated. When the URL contains a value for "nested.nestedField", the constructor argument "nested" should be populated.

[rstoyanchev]

I have pushed a fix. Effectively, we now skip nested constructor binding if there are no request parameters related to that argument. I've verified with both samples but feel free to test in your own environment as well.

[felixscheinost]

I can confirm that in my example project I can no longer reproduce the issue using 6.1.2. Thanks!