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.

Sign up for GitHub

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

Add @QueryMap mapEncoder attribute #1013

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add @QueryMap mapEncoder attribute #1013

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4a62e6d
Add @QueryMap mapEncoder attribute
galaxy-sea Apr 1, 2024
a971368
update license
galaxy-sea Apr 18, 2024
c1b27a8
SpringMapEncoder Change to @BaseQueryMapEncoder
galaxy-sea May 3, 2024
59e77ce
format code
galaxy-sea May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions ...ign-core/src/main/java/org/springframework/cloud/openfeign/FeignClientsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.springframework.cloud.openfeign.support.ResponseEntityDecoder;
import org.springframework.cloud.openfeign.support.SpringDecoder;
import org.springframework.cloud.openfeign.support.SpringEncoder;
import org.springframework.cloud.openfeign.support.SpringMapEncoder;
import org.springframework.cloud.openfeign.support.SpringMvcContract;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Conditional;
Expand All @@ -74,6 +75,7 @@
* @author Olga Maciaszek-Sharma
* @author Hyeonmin Park
* @author Yanming Zhou
* @author changjin wei(魏昌进)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update the year in license comments of all the modified files to -2024.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

*/
@Configuration(proxyBeanMethods = false)
public class FeignClientsConfiguration {
Expand Down Expand Up @@ -144,9 +146,9 @@ public QueryMapEncoder feignQueryMapEncoderPageable() {

@Bean
@ConditionalOnMissingBean
public Contract feignContract(ConversionService feignConversionService) {
public Contract feignContract(ConversionService feignConversionService, List<SpringMapEncoder> springMapEncoders) {
boolean decodeSlash = feignClientProperties == null || feignClientProperties.isDecodeSlash();
return new SpringMvcContract(parameterProcessors, feignConversionService, decodeSlash);
return new SpringMvcContract(parameterProcessors, feignConversionService, decodeSlash, springMapEncoders);
}

@Bean
Expand Down
11 changes: 11 additions & 0 deletions ...loud-openfeign-core/src/main/java/org/springframework/cloud/openfeign/SpringQueryMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,26 @@
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

import org.springframework.cloud.openfeign.support.SpringMapEncoder;

/**
* Spring MVC equivalent of OpenFeign's {@link feign.QueryMap} parameter annotation.
*
* @author Aram Peres
* @author changjin wei(魏昌进)
* @see feign.QueryMap
* @see org.springframework.cloud.openfeign.annotation.QueryMapParameterProcessor
*/
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.PARAMETER })
public @interface SpringQueryMap {

/**
* Specifies the {@link feign.QueryMapEncoder} implementation to use to transform DTO
* into query map. The {@link SpringMapEncoder} must be a valid spring bean.
* @return the {@link SpringMapEncoder} containing the instance of
* {@link feign.QueryMapEncoder}
*/
Class<? extends SpringMapEncoder> mapEncoder() default SpringMapEncoder.class;

}
24 changes: 24 additions & 0 deletions .../main/java/org/springframework/cloud/openfeign/annotation/QueryMapParameterProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,23 +18,37 @@

import java.lang.annotation.Annotation;
import java.lang.reflect.Method;
import java.util.Map;

import feign.MethodMetadata;
import feign.QueryMapEncoder;

import org.springframework.cloud.openfeign.AnnotatedParameterProcessor;
import org.springframework.cloud.openfeign.SpringQueryMap;
import org.springframework.cloud.openfeign.support.SpringMapEncoder;

/**
* {@link SpringQueryMap} parameter processor.
*
* @author Aram Peres
* @author Olga Maciaszek-Sharma
* @author changjin wei(魏昌进)
* @see AnnotatedParameterProcessor
*/
public class QueryMapParameterProcessor implements AnnotatedParameterProcessor {

private static final Class<SpringQueryMap> ANNOTATION = SpringQueryMap.class;

private final Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> encoders;

public QueryMapParameterProcessor() {
this.encoders = Map.of();
}

public QueryMapParameterProcessor(Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> encoders) {
this.encoders = encoders;
}

@Override
public Class<? extends Annotation> getAnnotationType() {
return ANNOTATION;
Expand All @@ -46,8 +60,18 @@ public boolean processArgument(AnnotatedParameterContext context, Annotation ann
MethodMetadata metadata = context.getMethodMetadata();
if (metadata.queryMapIndex() == null) {
metadata.queryMapIndex(paramIndex);
metadata.queryMapEncoder(getQueryMapEncoder(annotation));
}
return true;
}

protected QueryMapEncoder getQueryMapEncoder(Annotation annotation) {
SpringQueryMap springQueryMap = (SpringQueryMap) annotation;
SpringMapEncoder encoder = encoders.get(springQueryMap.mapEncoder());
if (encoder != null) {
return encoder.getQueryMapEncoder();
}
return null;
}

}
36 changes: 36 additions & 0 deletions ...eign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMapEncoder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright 2016-2024 the original author or authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.cloud.openfeign.support;

import feign.QueryMapEncoder;

/**
* Specifies the {@link QueryMapEncoder} implementation to use to transform DTO into query
* map.
*
* @author changjin wei(魏昌进)
* @see org.springframework.cloud.openfeign.SpringQueryMap
*/
public interface SpringMapEncoder {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why add another interface instead of just looking for QueryMapEncoder beans available in the context; what was the reasoning behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If using QueryMapEncoder, it will cause FeignClientFactoryBean to be unable to obtain the global QueryMapEncoder

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'm thinking we could have the users mark that one as @Primary if there's more than one, but that would be a braking change, so for the 2025.0.x release, but adding another interface feels redundant. Will discuss it with the team.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2025.0.x is also too far away😭. Still hope to merge PR as soon as possible

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion with the team, we would not like to add an additional interface. I think we should add a boolean opt-in flag and an additional annotation such as @BaseQueryMapEncoder to set the base builder one. Like this, if someone does not turn on the flag, things work for them as they did before; if they turn on the flag and have a bean annotated as @BaseQueryMapEncoder, that one is going to be used in the base builder; if they have beans without that annotation, those will only be used as possible to pick with the annotation, regardless of whether there's one or more. If you have a different idea, but without adding a wrapping interface, feel free to suggest it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @OlgaMaciaszek , I seem to have encountered some trouble.
I am unable to filter out bean that use @BaseQueryMapEncoder in multiple beans.

Map<String, QueryMapEncoder> queryMapEncoders = getInheritedAwareInstances(context, QueryMapEncoder.class);
    QueryMapEncoder queryMapEncoder = queryMapEncoders.values()
    .stream()
    .filter(encoder -> {
        // TODO: Filter QueryMapEncoders that use @BaseQueryMapEncoder
    })
    .findAny()
    .orElseGet(() -> getInheritedAwareOptional(context, QueryMapEncoder.class));

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this will require adding a change in SC Commons as well; Have created a PR: spring-cloud/spring-cloud-commons#1352, and then you'll need to add a method to handle the "inherited" flag, as in getInheritedAwareInstances method.


/**
* Returns the {@link QueryMapEncoder} implementation.
* @return a {@link QueryMapEncoder} instance.
*/
QueryMapEncoder getQueryMapEncoder();

}
23 changes: 20 additions & 3 deletions ...ign-core/src/main/java/org/springframework/cloud/openfeign/support/SpringMvcContract.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@
* @author Darren Foong
* @author Ram Anaswara
* @author Sam Kruglov
* @author changjin wei(魏昌进)
*/
public class SpringMvcContract extends Contract.BaseContract implements ResourceLoaderAware {

Expand Down Expand Up @@ -129,10 +130,16 @@ public SpringMvcContract(List<AnnotatedParameterProcessor> annotatedParameterPro

public SpringMvcContract(List<AnnotatedParameterProcessor> annotatedParameterProcessors,
ConversionService conversionService, boolean decodeSlash) {
this(annotatedParameterProcessors, conversionService, decodeSlash, List.of());
}

public SpringMvcContract(List<AnnotatedParameterProcessor> annotatedParameterProcessors,
ConversionService conversionService, boolean decodeSlash, List<SpringMapEncoder> springMapEncoders) {
Assert.notNull(annotatedParameterProcessors, "Parameter processors can not be null.");
Assert.notNull(conversionService, "ConversionService can not be null.");

List<AnnotatedParameterProcessor> processors = getDefaultAnnotatedArgumentsProcessors();
Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> encoders = toSpringMapEncodersMap(springMapEncoders);
List<AnnotatedParameterProcessor> processors = getDefaultAnnotatedArgumentsProcessors(encoders);
processors.addAll(annotatedParameterProcessors);

annotatedArgumentProcessors = toAnnotatedArgumentProcessorMap(processors);
Expand Down Expand Up @@ -363,15 +370,16 @@ private Map<Class<? extends Annotation>, AnnotatedParameterProcessor> toAnnotate
return result;
}

private List<AnnotatedParameterProcessor> getDefaultAnnotatedArgumentsProcessors() {
private List<AnnotatedParameterProcessor> getDefaultAnnotatedArgumentsProcessors(
Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> encoders) {

List<AnnotatedParameterProcessor> annotatedArgumentResolvers = new ArrayList<>();

annotatedArgumentResolvers.add(new MatrixVariableParameterProcessor());
annotatedArgumentResolvers.add(new PathVariableParameterProcessor());
annotatedArgumentResolvers.add(new RequestParamParameterProcessor());
annotatedArgumentResolvers.add(new RequestHeaderParameterProcessor());
annotatedArgumentResolvers.add(new QueryMapParameterProcessor());
annotatedArgumentResolvers.add(new QueryMapParameterProcessor(encoders));
annotatedArgumentResolvers.add(new RequestPartParameterProcessor());
annotatedArgumentResolvers.add(new CookieValueParameterProcessor());

Expand Down Expand Up @@ -415,6 +423,15 @@ private boolean isMultipartFormData(MethodMetadata data) {
return false;
}

public Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> toSpringMapEncodersMap(
List<SpringMapEncoder> springMapEncoders) {
Map<Class<? extends SpringMapEncoder>, SpringMapEncoder> result = new HashMap<>();
for (SpringMapEncoder encoder : springMapEncoders) {
result.put(encoder.getClass(), encoder);
}
return result;
}

private static class ConvertingExpanderFactory {

private final ConversionService conversionService;
Expand Down
3 changes: 2 additions & 1 deletion ...-openfeign-core/src/test/java/org/springframework/cloud/openfeign/SpringDecoderTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,15 @@
* Tests for {@link SpringDecoder}.
*
* @author Olga Maciaszek-Sharma
* @author changjin wei(魏昌进)
*/
class SpringDecoderTests {

SpringDecoder decoder;

@BeforeEach
void setUp() {
ObjectFactory<HttpMessageConverters> factory = mock();
ObjectFactory<HttpMessageConverters> factory = mock(ObjectFactory.class);
when(factory.getObject()).thenReturn(new HttpMessageConverters());
decoder = new SpringDecoder(factory);
}
Expand Down
40 changes: 40 additions & 0 deletions ...ore/src/test/java/org/springframework/cloud/openfeign/support/SpringMvcContractTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.fasterxml.jackson.annotation.JsonAutoDetect;
import feign.MethodMetadata;
import feign.Param;
import feign.QueryMapEncoder;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand Down Expand Up @@ -82,6 +83,7 @@
* @author Szymon Linowski
* @author Sam Kruglov
* @author Bhavya Agrawal
* @author changjin wei(魏昌进)
**/

class SpringMvcContractTests {
Expand Down Expand Up @@ -548,6 +550,22 @@ void testProcessQueryMapObject() throws Exception {
assertThat(params.get("aParam").iterator().next()).isEqualTo("{aParam}");
}

@Test
void testProcessQueryMapEncoder() throws Exception {
contract = new SpringMvcContract(Collections.emptyList(), getConversionService(), true,
List.of(new TestSpringMapEncoder()));
Method method = TestTemplate_QueryMap.class.getDeclaredMethod("queryMapEncoder", TestObject.class,
String.class);
MethodMetadata data = contract.parseAndValidateMetadata(method.getDeclaringClass(), method);

assertThat(data.template().url()).isEqualTo("/queryMapEncoder?aParam=" + "{aParam}");
assertThat(data.template().method()).isEqualTo("GET");
assertThat(data.queryMapIndex().intValue()).isEqualTo(0);
assertThat(data.queryMapEncoder().getClass()).isEqualTo(TestQueryMapEncoder.class);
Map<String, Collection<String>> params = data.template().queries();
assertThat(params.get("aParam").iterator().next()).isEqualTo("{aParam}");
}

@Test
void testProcessQueryMapMoreThanOnce() throws Exception {
Method method = TestTemplate_QueryMap.class.getDeclaredMethod("queryMapMoreThanOnce", MultiValueMap.class,
Expand Down Expand Up @@ -775,6 +793,10 @@ String queryMapMoreThanOnce(@RequestParam MultiValueMap<String, String> queryMap
@GetMapping("/queryMapObject")
String queryMapObject(@SpringQueryMap TestObject queryMap, @RequestParam(name = "aParam") String aParam);

@GetMapping("/queryMapEncoder")
String queryMapEncoder(@SpringQueryMap(mapEncoder = TestSpringMapEncoder.class) TestObject queryMap,
@RequestParam(name = "aParam") String aParam);

}

public interface TestTemplate_RequestPart {
Expand Down Expand Up @@ -920,4 +942,22 @@ public String toString() {

}

public static class TestSpringMapEncoder implements SpringMapEncoder {

@Override
public QueryMapEncoder getQueryMapEncoder() {
return new TestQueryMapEncoder();
}

}

public static class TestQueryMapEncoder implements QueryMapEncoder {

@Override
public Map<String, Object> encode(Object object) {
return Map.of("foo", "foo");
}

}

}
Loading