Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update RequestParamParameterProcessor to support map param #1102

Closed
wants to merge 3 commits into from

Conversation

lowzj
Copy link
Contributor

@lowzj lowzj commented Jun 15, 2016

see issue #1097

@lowzj
Copy link
Contributor Author

lowzj commented Jun 15, 2016

travis-ci build error, since travis-ci job #1353:

[ERROR] MavenReportException: Error while generating Javadoc: 
Exit code: 1 - /home/travis/.m2/repository/com/netflix/eureka/eureka-client/1.4.8/eureka-client-1.4.8.jar(com/netflix/discovery/DiscoveryClient.class): warning: Cannot find annotation method 'name()' in type 'Monitor': class file for com.netflix.servo.annotations.Monitor not found
...
[ERROR] Error fetching link: /home/travis/build/spring-cloud/spring-cloud-netflix/spring-cloud-netflix-eureka-client/target/apidocs/package-list. Ignored it.
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-source-plugin:3.0.0:jar (attach-sources) on project spring-cloud-netflix-eureka-server: Error creating source archive: Problem creating jar: Execution exception: Java heap space -> [Help 1]

@spencergibb
Copy link
Member

yeah, we had to revert a fix for travis, don't worry about that.

@lowzj lowzj force-pushed the request-param-processor branch from 248d31d to 88b466b Compare June 22, 2016 00:52
@spencergibb
Copy link
Member

@lowzj any chance you could add some tests?

@lowzj lowzj force-pushed the request-param-processor branch from 88b466b to 3617991 Compare June 29, 2016 12:58
@lowzj
Copy link
Contributor Author

lowzj commented Jun 29, 2016

At first I run the test I wrote in this pr, it passed. Then I recompile the whole project, and test again, it failed. I create a new spring-boot project, include the spring-cloud-netflix-core dependency and run the same test again, it passed.

I debug the test and find the problem in my case, and I think probably there is something wrong in the method SpringMvcContract#synthesizeWithMethodParameterNameAsFallbackValue.

private Annotation synthesizeWithMethodParameterNameAsFallbackValue(
    Annotation parameterAnnotation, Method method, int parameterIndex) {
    ...
    String[] parameterNames = PARAMETER_NAME_DISCOVERER.getParameterNames(method);
    ...
}

The code above finally invokes the method java.lang.reflect.Executable#getParameters --> privateGetParameters --> getParameters0.
I'm not sure about the mechanism of java.lang.reflect.Executable#getParameters0, I find that it doesn't always return the expected result.

    public interface TestTemplate_MapParams {
        @RequestMapping(value = "/test", method = RequestMethod.GET)
        ResponseEntity<TestObject> getTest(@RequestParam Map<String, String> params);
    }

In this case , parameterNames expected result is ["params"].
In this project, getParameters0 return ["params"], and it's according to the class file. It's the expected result.
When I start a new project using the spring-cloud-netflix-core, getParameters0 return null then Executable synthesize all parameters' names('arg'+parameterIndex) and return ["arg0"]. arg0 is not according to the class file, so parameterNames would be null. So there is no fallback value for the annotation name/value attributes.

I haven't resolve this problem(may be not) now, but I think that only a simple property need a fallback value , such as String, Long, int... and so on.

@lowzj
Copy link
Contributor Author

lowzj commented Jun 30, 2016

Finally, I find that java.lang.reflect.Executable#getParameters0 depends on compiler options

$ javac -help
...
-parameters                Generate metadata for reflection on method parameters
...

Then I add these code in my own project pom.xml

<build>
    <plugins>
        <plugin>
            <groupId>org.apache.maven.plugins</groupId>
            <artifactId>maven-compiler-plugin</artifactId>
            <configuration>
                <compilerArgs>
                    <arg>-parameters</arg>
                </compilerArgs>
            </configuration>
        </plugin>
    </plugins>
</build>

the method java.lang.reflect.Executable#getParameters0 return the expected result.

@lowzj
Copy link
Contributor Author

lowzj commented Jun 30, 2016

java documents explain my problem: Obtaining Names of Method Parameters

@spencergibb
Copy link
Member

So -parameters is java 8 functionality and, except for a module or two, we are still baselined at java 7.

@lowzj
Copy link
Contributor Author

lowzj commented Jul 1, 2016

thanks, I got it.

@lowzj
Copy link
Contributor Author

lowzj commented Jul 3, 2016

@spencergibb Is there any suggestion about this pr?

@spencergibb
Copy link
Member

@lowzj if this requires java 8 we won't be able to merge.

@lowzj
Copy link
Contributor Author

lowzj commented Aug 18, 2016

@spencergibb it support java 7/8. I modify the SpringMvcContract#synthesizeWithMethodParameterNameAsFallbackValue to just affect simple properties.

spencergibb added a commit that referenced this pull request Aug 31, 2016
* lowzj-request-param-processor:
  update RequestParamParameterProcessor to support map param
@spencergibb
Copy link
Member

Squashed and merged with a bit of polish via dc26b1e

@lynchlee
Copy link

lynchlee commented Sep 1, 2016

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants