Spring Constructor based bean binding should support package protected constructors and accessors

The constructor based bean binding currently only supports public constructors. If a constructor is declared package private, this is the primary error message one sees:

java.lang.NoSuchMethodException: com.acme.MyForm.<init>()

This is misleading in itself as it seems to indicate that a default constructor is needed, when in fact the visibility of the declared constructor is the issue.

Also, non-public accessor methods will cause property lookups to fail, e.g. a package protected getter for name:

Caused by: org.springframework.beans.NotReadablePropertyException: Invalid property 'name' of bean class [com.acme.MyForm]: Bean property 'name' is not readable or has an invalid getter method: Does the return type of the getter match the parameter type of the setter?

Again, the error message is misleading as it points to the return type, not the visibility.

It would be cool if package protected constructors and accessor were supported out of the box. If you decide to end up not supporting those, a hint at the requirement to declare them public in the error messages would be very helpful.

Comment From: sbrannen

@odrotbohm, are you referring to the support introduced in #19763?

Comment From: odrotbohm

Yes. You can find an executable example here. Steps to reproduce:

$ git clone https://github.com/st-tu-dresden/guestbook
cd guestbook
mvn spring-boot:run
See the form display and submission working.

Scenario one (constructor parameter visibility):

Remove public from GuestbookForm's constructor.
Restart the app.
See the first exception thrown when you submit the form.

Scenario two (getter method visibility):

Remove public from GuestbookForm's getters.
Restart the app.
See the first request to display the entries already failing with the second exception message above coming from Spring's property accessors.

Comment From: sbrannen

Thanks for providing the sample application, @odrotbohm!

We'll look into it.

By the way, the application throws the following exception when started as is.

java.lang.NullPointerException: null
    at org.springframework.http.MediaType.parseMediaType(MediaType.java:538) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.http.MediaTypeFactory.parseMimeTypes(MediaTypeFactory.java:76) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.http.MediaTypeFactory.<clinit>(MediaTypeFactory.java:47) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.accept.PathExtensionContentNegotiationStrategy.getMediaTypeForResource(PathExtensionContentNegotiationStrategy.java:118) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.accept.ServletPathExtensionContentNegotiationStrategy.getMediaTypeForResource(ServletPathExtensionContentNegotiationStrategy.java:104) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.getMediaType(ResourceHttpRequestHandler.java:668) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.handleRequest(ResourceHttpRequestHandler.java:476) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter.handle(HttpRequestHandlerAdapter.java:53) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1038) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:897) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:634) ~[tomcat-embed-core-9.0.16.jar:9.0.16]
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:882) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]

And after processing the first form submission:

2019-03-18 12:11:42.698 ERROR 75005 --- [nio-8080-exec-1] o.a.c.c.C.[.[.[/].[dispatcherServlet]    : Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Handler dispatch failed; nested exception is java.lang.NoClassDefFoundError: Could not initialize class org.springframework.http.MediaTypeFactory] with root cause

java.lang.NoClassDefFoundError: Could not initialize class org.springframework.http.MediaTypeFactory
    at org.springframework.web.accept.PathExtensionContentNegotiationStrategy.getMediaTypeForResource(PathExtensionContentNegotiationStrategy.java:118) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.accept.ServletPathExtensionContentNegotiationStrategy.getMediaTypeForResource(ServletPathExtensionContentNegotiationStrategy.java:104) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.getMediaType(ResourceHttpRequestHandler.java:668) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.resource.ResourceHttpRequestHandler.handleRequest(ResourceHttpRequestHandler.java:476) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter.handle(HttpRequestHandlerAdapter.java:53) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1038) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:897) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:634) ~[tomcat-embed-core-9.0.16.jar:9.0.16]
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:882) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]

Are you aware of those?

Is that a bug that's already been reported, or shall we investigate that as well?

Comment From: sbrannen

Scenario one (constructor parameter visibility):

1. Remove `public` from [`GuestbookForm`'s constructor](https://github.com/st-tu-dresden/guestbook/blob/d2ca04524a811f73febd73cb04f2858658601e11/src/main/java/guestbook/GuestbookForm.java#L43).

2. Restart the app.

3. See the first exception thrown when you submit the form.

Indeed, this is what we see:

java.lang.NoSuchMethodException: guestbook.GuestbookForm.<init>()
    at java.base/java.lang.Class.getConstructor0(Class.java:3350) ~[na:na]
    at java.base/java.lang.Class.getDeclaredConstructor(Class.java:2554) ~[na:na]
    at org.springframework.web.method.annotation.ModelAttributeMethodProcessor.createAttribute(ModelAttributeMethodProcessor.java:216) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.method.annotation.ServletModelAttributeMethodProcessor.createAttribute(ServletModelAttributeMethodProcessor.java:84) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.method.annotation.ModelAttributeMethodProcessor.resolveArgument(ModelAttributeMethodProcessor.java:139) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:126) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:166) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:134) ~[spring-web-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:102) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:895) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:800) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1038) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:897) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:634) ~[tomcat-embed-core-9.0.16.jar:9.0.16]
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:882) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]

However, the Javadoc for ModelAttributeMethodProcessor.createAttribute(...) states the following.

The default implementation typically uses the unique public no-arg constructor if available but also handles a "primary constructor" approach for data classes: It understands the JavaBeans ConstructorProperties annotation as well as runtime-retained parameter names in the bytecode, associating request parameters with constructor arguments by name. If no such constructor is found, the default constructor will be used (even if not public), assuming subsequent bean property bindings through setter methods.

Consequently, I would also expect the algorithm to support a non-public "primary constructor" for data classes.

@jhoeller, do you agree with that? Also, if you're not actively working on this, I can put together a fix.

Comment From: sbrannen

Scenario two (getter method visibility):

1. Remove `public` from [`GuestbookForm`'s getters](https://github.com/st-tu-dresden/guestbook/blob/d2ca04524a811f73febd73cb04f2858658601e11/src/main/java/guestbook/GuestbookForm.java#L49-L69).

2. Restart the app.

3. See the first request to display the entries already failing with the second exception message above coming from Spring's property accessors.

To simplify later analysis, I am posting the stacktrace for this scenario as well.

2019-03-18 12:25:45.592 ERROR 75320 --- [nio-8080-exec-1] o.a.c.c.C.[.[.[/].[dispatcherServlet]    : Servlet.service() for servlet [dispatcherServlet] in context with path [] threw exception [Request processing failed; nested exception is org.thymeleaf.exceptions.TemplateInputException: An error happened during template parsing (template: "class path resource [templates/guestbook.html]")] with root cause

org.springframework.beans.NotReadablePropertyException: Invalid property 'name' of bean class [guestbook.GuestbookForm]: Bean property 'name' is not readable or has an invalid getter method: Does the return type of the getter match the parameter type of the setter?
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:622) ~[spring-beans-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.beans.AbstractNestablePropertyAccessor.getPropertyValue(AbstractNestablePropertyAccessor.java:612) ~[spring-beans-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.validation.AbstractPropertyBindingResult.getActualFieldValue(AbstractPropertyBindingResult.java:104) ~[spring-context-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.validation.AbstractBindingResult.getFieldValue(AbstractBindingResult.java:228) ~[spring-context-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.support.BindStatus.<init>(BindStatus.java:129) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.support.RequestContext.getBindStatus(RequestContext.java:903) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.thymeleaf.spring5.context.webmvc.SpringWebMvcThymeleafRequestContext.getBindStatus(SpringWebMvcThymeleafRequestContext.java:227) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.util.FieldUtils.getBindStatusFromParsedExpression(FieldUtils.java:306) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.util.FieldUtils.getBindStatus(FieldUtils.java:253) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.util.FieldUtils.getBindStatus(FieldUtils.java:227) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.processor.AbstractSpringFieldTagProcessor.doProcess(AbstractSpringFieldTagProcessor.java:174) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.processor.element.AbstractAttributeTagProcessor.doProcess(AbstractAttributeTagProcessor.java:74) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.processor.element.AbstractElementTagProcessor.process(AbstractElementTagProcessor.java:95) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.util.ProcessorConfigurationUtils$ElementTagProcessorWrapper.process(ProcessorConfigurationUtils.java:633) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.engine.ProcessorTemplateHandler.handleStandaloneElement(ProcessorTemplateHandler.java:918) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.engine.TemplateHandlerAdapterMarkupHandler.handleStandaloneElementEnd(TemplateHandlerAdapterMarkupHandler.java:260) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.templateparser.markup.InlinedOutputExpressionMarkupHandler$InlineMarkupAdapterPreProcessorHandler.handleStandaloneElementEnd(InlinedOutputExpressionMarkupHandler.java:256) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.standard.inline.OutputExpressionInlinePreProcessorHandler.handleStandaloneElementEnd(OutputExpressionInlinePreProcessorHandler.java:169) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.templateparser.markup.InlinedOutputExpressionMarkupHandler.handleStandaloneElementEnd(InlinedOutputExpressionMarkupHandler.java:104) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.attoparser.HtmlElement.handleStandaloneElementEnd(HtmlElement.java:79) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.HtmlMarkupHandler.handleStandaloneElementEnd(HtmlMarkupHandler.java:241) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.MarkupEventProcessorHandler.handleStandaloneElementEnd(MarkupEventProcessorHandler.java:327) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.ParsingElementMarkupUtil.parseStandaloneElement(ParsingElementMarkupUtil.java:96) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.MarkupParser.parseBuffer(MarkupParser.java:706) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.MarkupParser.parseDocument(MarkupParser.java:301) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.attoparser.MarkupParser.parse(MarkupParser.java:257) ~[attoparser-2.0.5.RELEASE.jar:2.0.5.RELEASE]
    at org.thymeleaf.templateparser.markup.AbstractMarkupTemplateParser.parse(AbstractMarkupTemplateParser.java:230) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.templateparser.markup.AbstractMarkupTemplateParser.parseStandalone(AbstractMarkupTemplateParser.java:100) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.engine.TemplateManager.parseAndProcess(TemplateManager.java:666) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.TemplateEngine.process(TemplateEngine.java:1098) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.TemplateEngine.process(TemplateEngine.java:1072) ~[thymeleaf-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.view.ThymeleafView.renderFragment(ThymeleafView.java:362) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.thymeleaf.spring5.view.ThymeleafView.render(ThymeleafView.java:189) ~[thymeleaf-spring5-3.0.11.RELEASE.jar:3.0.11.RELEASE]
    at org.springframework.web.servlet.DispatcherServlet.render(DispatcherServlet.java:1370) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.processDispatchResult(DispatcherServlet.java:1116) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1055) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:942) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1005) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at org.springframework.web.servlet.FrameworkServlet.doGet(FrameworkServlet.java:897) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:634) ~[tomcat-embed-core-9.0.16.jar:9.0.16]
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:882) ~[spring-webmvc-5.2.0.BUILD-SNAPSHOT.jar:5.2.0.BUILD-SNAPSHOT]
    at javax.servlet.http.HttpServlet.service(HttpServlet.java:741) ~[tomcat-embed-core-9.0.16.jar:9.0.16]

After some analysis, it would appear that the non-public getter method is not supported since ExtendedBeanInfo.isCandidateWriteMethod(Method) looks for a public setter method to determine (infer?) if the bean property has a readable getter method.

@jhoeller, do you also agree with addressing this issue? If so, I would say scenario # 1 is a bug; whereas, scenario # 2 would be an enhancement.

Comment From: odrotbohm

Is that a bug that's already been reported, or shall we investigate that as well?

I cannot reproduce that locally. The CI build seems to be fine and a local retry also worked. Do you have pending changes locally maybe?

I understand where the current limitations are coming from. It just feels a bit inconsistent from a user's point of view: if she decides to make the class package protected in the first place, it feels awkward being "forced" to make methods public. FWIW, the type can be used by the framework, hence the latter is operating as if it was code living inside the very package. If that's the case in the first place, it should be able to use those constructors and methods without additionally making them public. Also, general Spring's general component handling supports constructors and accessors of reduced visibility so that the lack of support for that in beans binding feels inconsistent.

I also understand that using those constructors and methods would introduce the need for more (read: costly) inspection. Maybe that additional mode can be activated only if the type itself is package private. Although that would render the combination public type, package protected accessors unsupported. Not sure if that is desirable, though.

Generally speaking, visibility decisions are something affecting application design and architecture significantly and I think the framework should try to avoid imposing constraints as much as possible.

Comment From: sbrannen

Is that a bug that's already been reported, or shall we investigate that as well?

I cannot reproduce that locally. The CI build seems to be fine and a local retry also worked. Do you have pending changes locally maybe?

It seems to have gone away on its own. No idea what was happening there...

I also understand that using those constructors and methods would introduce the need for more (read: costly) inspection.

Regarding the constructors, I don't think it would be that costly: we already look for a default constructor; whereas, we could change the algorithm to look for a sole constructor (regardless of whether it accepts arguments) and use that even if it's not public.

Regarding the methods, we already cache the introspection in CachedIntrospectionResults. So, removing the "only public JavaBeans properties" check wouldn't be very costly.

Maybe that additional mode can be activated only if the type itself is package private.

We might indeed need to be careful about implementing this change, since it would technically be a breaking change for anyone who relied on the fact that non-public getters and setters would not be candidates for bean binding from web requests.

Although that would render the combination public type, package protected accessors unsupported. Not sure if that is desirable, though.

Agreed: making certain "combinations" would likely be undesirable. I think it's best to have a consistent story/algorithm.

Generally speaking, visibility decisions are something affecting application design and architecture significantly and I think the framework should try to avoid imposing constraints as much as possible.

I agree.

@jhoeller, what are your thoughts on removing the current restrictions?

Comment From: odrotbohm

I ran into this again. Any further updates? Having this fixed in 5.2 would be highly appreciated. 🙃

Comment From: rob-valor

Hi Spring Team, first time for me here so sorry if my question does not really belongs here.

I was searching for ModelAttributeMethodProcessor in the issues list and this issue is somewhat related. We use constructor injection of 'url' data for our attribute (created by ModelAttributeMethodProcessor.createAttribute(...). Trying to use immutable objects (Lombok @Value) we face the problem that only request parameters are taken into account for constructor injection and not the path variables. Because of that we currently need a mutable object. Is there a reason to not support path variables? Maybe I should report this as an actual issue, but since this issue is related to the attributes' constructor i thought I could start my journey here.

Comment From: odrotbohm

I think what data is available for binding has not changed really. Are you saying that you get the path variables bound automatically if you add setters to the type to bind to? If you bind them manually, adding with…(…) methods could allow staying with an immutable type but augmenting it with additional data in the controller method.

Comment From: rob-valor

I also think this has not changed recently and yes, path variables (name:value pairs) are explicitly added for bean property binding.

In method constructAttribute (ModelAttributeMethodProcessor.java:277) the values for constructor parameters are only taken from webRequest.getParameterValues(paramName) so no path variables are taken into account here. It's only when that constructed attribute object is bind by bindRequestParameters(binder, webRequest) in method resolveArgument (ModelAttributeMethodProcessor.java:160) that the path variables (as well as the request parameters again) are eventually added for bean property binding in method ServletRequestDataBinder#bind.

So to me it looks like an inconsistent support for injecting request url variables into the attribute object constructor. Or this is explicitly designed that way but to me not clear why.

Comment From: rstoyanchev

@rob-valor I agree it is inconsistent that it is possible to bind path variables via setters but not via constructor args. However that is not relevant for the issue here and is mixing independent conversation threads. Can you please create a separate issue with a description summarizing your comments above?

Comment From: jhoeller

On review, I'm rather opposed to changing our policy here since public visibility is a way of controlling what is exposed for data binding and what is not. Also, JavaBeans introspection itself is entirely focused on public accessors, as are Java 14/15 records. Constructor-based binding seems to be in sync with setter-based binding there, so at least we don't have any inconsistencies there as far as I can see. That said, point taken that we should improve our exception messages there.

Comment From: odrotbohm

public visibility is a way of controlling what is exposed for data binding

I kind of disagree here as which visibility is chosen for properties and constructors might be completely driven by other aspects. If certain properties are not supposed to be bound, there's the binder configuration that can be used. Also, in my example above the class mentioned in my original example is package protected in the first place but still subject for data binding.

IMO the primary inconsistency is in the fact that a package protected class needs a public constructor which doesn't make a lot of sense to add because the type is logically visible in the same package only anyway. The implied rationale is that the framework acts from "the outside" but why does it consider the type then in the first place?

Also, in all other cases in which the framework invokes constructors (e.g. bean instances) it behaves as if it was code located in the same package. It's totally not obvious why it should behave differently here. Also, the fundamental APIs backing all of this (BeanUtils.instantiateClass(Constructor, Object...) happy uses package protected constructors as it makes them accessible prior to instantiation.