Skip to content

Conversation

@remeio
Copy link
Contributor

@remeio remeio commented Jan 4, 2025

Keep the - of header names when init parameter names, otherwise the nested path will not work.

@rstoyanchev Hi, for property name and parameter name, I think they should use the same transformed header name using "camelCase".

For example, A http request with headers:

  • User-Agent: something
  • WWW-Authenticate: something

Before this PR, the method ExtendedServletRequestValueResolver#initParameterNames will return a name set:

  • UserAgent, WWWAuthenticate

After this PR, the method will return a name set:

  • userAgent, wWWAuthenticate

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jan 4, 2025
@remeio remeio changed the title Transform header name to property name using "camelCase" Transform header name same with property name Jan 4, 2025
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Jan 6, 2025
@bclozel bclozel self-assigned this Jan 7, 2025
@bclozel
Copy link
Member

bclozel commented Jan 8, 2025

@remeio Can you elaborate on which use case you're trying to solve here?
Could you share a sample controller and data class to be bound to explain what's missing here?

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jan 8, 2025
@remeio remeio force-pushed the transform_header_name branch from efbdf84 to 89a90f1 Compare January 8, 2025 15:37
@remeio remeio changed the title Transform header name same with property name Keep the - of header on init parameter names, otherwise the nested path will not work. Jan 8, 2025
@remeio
Copy link
Contributor Author

remeio commented Jan 8, 2025

@remeio Can you elaborate on which use case you're trying to solve here? Could you share a sample controller and data class to be bound to explain what's missing here?

@bclozel Hi, I had changed the content of this PR , you can look the test case.

I think it's not correct for replace the - of header names when init parameter names, because the nested path will not work.

@bclozel
Copy link
Member

bclozel commented Jan 8, 2025

I missed the fact that this was for nested parameters, indeed. Thanks for clarifying that and adding a test case.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 8, 2025
@bclozel bclozel added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 8, 2025
@bclozel bclozel modified the milestone: 6.2.2 Jan 8, 2025
@bclozel bclozel added status: waiting-for-triage An issue we've not yet triaged or decided on and removed type: bug A general bug labels Jan 8, 2025
@bclozel
Copy link
Member

bclozel commented Jan 8, 2025

I'm not sure about this change after all.

When binding bean properties, the following case will work:

class Person() {

  // will bind HTTP header "First-Name" : "Jane"
  String firstName;

  public String getFirstName() {
    //
  }
  public String setFirstName(String firstName) {
    //
  }
}

Constructor binding requires the @BindParam for the same behavior

// will bind HTTP header "First-Name" : "Jane"
record Person(@BindParam("First-Name") String firstName) {
}

This PR is about supporting the nested case like so:

record MyAddress(@BindParam("My-Address") Address myAddress) {
}

// will bind HTTP header "My-Address.Street-Name" : "Spring Street"
record Address(@BindParam("Street-Name") String streetName) {
}

The ABNF of RFC7230 allows "." in header names:

header-field = field-name ":" OWS field-value OWS
field-name = token
token = 1tchar
tchar = "!" / "#" / "$" / "%" / "&" / "'" / "
" / "+" / "-" / "." /
"^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

But in practice, I'm wondering if we should rely on "." characters in header names. Proxies and CDNs might not accept it.

What do you think @rstoyanchev ?

@bclozel bclozel added the status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team label Jan 8, 2025
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 27, 2025

Indeed, in #32676 we didn't mean to make headers a mechanism for binding to any object structure. The main intent was to include headers in data binding, and those are usually simple, non-hierarchical names. It's similar to what we do for path variables.

@remeio is there a specific case with such headers in use that you've come across?

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-internal-feedback An issue that needs input from a member or another Spring Team labels Jan 27, 2025
@remeio
Copy link
Contributor Author

remeio commented Jan 31, 2025

Indeed, in #32676 we didn't mean to make headers a mechanism for binding to any object structure. The main intent was to include headers in data binding, and those are usually simple, non-hierarchical names. It's similar to what we do for path variables.

@remeio is there a specific case with such headers in use that you've come across?

Hi, There don't have the specific case. I commit this PR just because my code review. In my opinion, I think the handle logic of header and param should be keep the same, it's nice for Spring user, not the coder.

You also can close this PR, it's OK.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 31, 2025
@rstoyanchev
Copy link
Contributor

Thanks for the feedback. In that case I will close this for now.

@rstoyanchev rstoyanchev added status: declined A suggestion or change that we don't feel we should currently apply and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Jan 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in: web Issues in web modules (web, webmvc, webflux, websocket) status: declined A suggestion or change that we don't feel we should currently apply

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants