Issue 1360 - Support header map and query map

[asarkar]

#1360

[pivotal-issuemaster]

@abhijitsarkar Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

[pivotal-issuemaster]

@abhijitsarkar Thank you for signing the Contributor License Agreement!

[codecov-io]

Current coverage is 73.75% (diff: 70.00%)

Merging #1361 into master will decrease coverage by 0.90%

@@             master      #1361   diff @@
==========================================
  Files           187        184     -3   
  Lines          5751       5659    -92   
  Methods           0          0          
  Messages          0          0          
  Branches        864        862     -2   
==========================================
- Hits           4294       4174   -120   
- Misses         1149       1182    +33   
+ Partials        308        303     -5   

Powered by Codecov. Last update 139ab95...b7498ae

[ryanjbaxter]

@spencergibb will probably have to take a look at this as well, but it looks good to me. Do you think we should add some documentation as well? Or since we are using Spring MVC annotations it should be obvious this is supported?

[asarkar]

@ryanjbaxter I'd think one of the benefits of using the Spring annotations is that people are already familiar with those and don't need further guidance. The behavior I implemented is already documented:

RequestHeader doc:

If the method parameter is Map<String, String>, MultiValueMap<String, String>, or HttpHeaders then the map is populated with all header names and values.

RequestParam doc:

If the method parameter is Map<String, String> or MultiValueMap<String, String> and a parameter name is not specified, then the map parameter is populated with all request parameter names and values.

[ryanjbaxter]

@abhijitsarkar I agree, just making sure we are all on the same page, thanks!

[asarkar]

@spencergibb @ryanjbaxter Do you need anything else from me? If not, when can I expect this PR to be merged?

[ryanjbaxter]

Just waiting for @spencergibb to take a look

[asarkar]

@ryanjbaxter Can someone else take a look? It's not much of a community driven project if one person becomes a bottleneck. There're other PRs open for months, neither accepted, nor rejected.

[spencergibb]

@abhijitsarkar No need to get testy. Half of the team is on leave, the other half is traveling for a conference. I'll look at it early next week.

[ryanjbaxter]

I am not trying to make anyone a bottleneck, I am the "new guy" on the team so I am a little more cautious merging commits in code I am not too familiar with. Sorry for the inconvenience.

[spencergibb]

Formatting: we use tabs, not spaces

[spencergibb]

Since this is not a backward compatible change on a public interface, I'm hesitant to make this change in a bugfix release. I've been bitten by this too often.

[asarkar]

@spencergibb

1. I can make the changes backward compatible by extending from the AnnotatedParameterProcessor instead of modifying it. That'll create 4 more files, which I intend to name Enhanced*. SpringMvcContract will use the new files by default and if there's anyone passing in custom AnnotatedParameterProcessor (I doubt it), their code won't break. What do you think?
2. I used the Eclipse code formatter plugin as instructed in the Code Conventions and Housekeeping, and of course, the CI build also didn't complain. How was I supposed to know that you use tabs, not spaces, if the build doesn't enforce it (using Checkstyle or something like that) and not documented either?

[spencergibb]

RE: 1) Do it, deprecate the originals and in the next release we will remove as for 2) the formatter should and I just told you.

[asarkar]

@spencergibb Requested changes in. Please review.

[asarkar]

Is there something else I can do to help this get merged?

[asarkar]

Following up to see what's the hold up and if anything if needed from me.

[spencergibb]

@abhijitsarkar I think I made a mistake in having you split the interface to try and make it backward compatible. It's too messy. If you're willing, I'd like to go back to the single interface for the next major version.

[asarkar]

@spencergibb I've reverted my changes on my branch and the PR seems to have picked it up. Feel free to merge at will.