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

RESTEasy Reactive: Handle separator for bean params #31784

Merged
merged 1 commit into from
Mar 31, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
package io.quarkus.resteasy.reactive.server.test;

import static org.junit.jupiter.api.Assertions.fail;

import jakarta.enterprise.inject.spi.DeploymentException;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.Path;

import org.jboss.resteasy.reactive.RestQuery;
import org.jboss.resteasy.reactive.RestResponse;
import org.jboss.resteasy.reactive.Separator;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import io.quarkus.test.QuarkusUnitTest;

public class SingleQueryParamWithSeparatorTest {

@RegisterExtension
static QuarkusUnitTest test = new QuarkusUnitTest()
.withApplicationRoot((jar) -> jar.addClass(TestResource.class))
.setExpectedException(DeploymentException.class);

@Test
public void test() {
fail("Should never have been called");
}

@Path("test")
public static class TestResource {

@GET
@Path("endpoint")
public RestResponse<String> endpoint(@RestQuery @Separator(",") String parameter) {
return RestResponse.ResponseBuilder.ok(parameter).build();
}

}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package io.quarkus.resteasy.reactive.server.test.simple;

import static io.restassured.RestAssured.*;
import static io.restassured.RestAssured.get;

import java.util.List;

Expand Down Expand Up @@ -69,6 +69,15 @@ public void multipleQueryParams() {
.header("x-size", "6");
}

@Test
public void multipleQueryParamsBean() {
get("/hello/bean?name=foo,bar&name=one,two,three&name=yolo")
.then()
.statusCode(200)
.body(Matchers.equalTo("hello foo bar one two three yolo"))
.header("x-size", "6");
}

@Path("hello")
public static class HelloResource {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,7 @@ public Optional<ResourceClass> createEndpoints(ClassInfo classInfo, boolean cons
clazz.setPath(sanitizePath(path));
}
if (factoryCreator != null) {
clazz.setFactory((BeanFactory<Object>) factoryCreator.apply(classInfo.name().toString()));
clazz.setFactory(factoryCreator.apply(classInfo.name().toString()));
}
Map<String, String> classLevelExceptionMappers = this.classLevelExceptionMappers.get(classInfo.name());
if (classLevelExceptionMappers != null) {
Expand Down Expand Up @@ -1238,10 +1238,12 @@ public PARAM extractParameterInfo(ClassInfo currentClassInfo, ClassInfo actualEn
} else if (queryParam != null) {
builder.setName(queryParam.value().asString());
builder.setType(ParameterType.QUERY);
builder.setSeparator(getSeparator(anns));
convertible = true;
} else if (restQueryParam != null) {
builder.setName(valueOrDefault(restQueryParam.value(), sourceName));
builder.setType(ParameterType.QUERY);
builder.setSeparator(getSeparator(anns));
convertible = true;
} else if (cookieParam != null) {
builder.setName(cookieParam.value().asString());
Expand Down Expand Up @@ -1438,6 +1440,9 @@ && isParameterContainerType(paramType.asClassType())) {
if (suspendedAnnotation != null && !elementType.equals(AsyncResponse.class.getName())) {
throw new RuntimeException("Can only inject AsyncResponse on methods marked @Suspended");
}
if (builder.isSingle() && builder.getSeparator() != null) {
throw new DeploymentException("Single parameters should not be marked with @Separator");
}
builder.setElementType(elementType);
return builder;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ public class IndexedParameter<T extends IndexedParameter<T>> {
protected String elementType;
protected boolean single;
protected boolean optional;
protected String separator;

public boolean isObtainedAsCollection() {
return !single
Expand Down Expand Up @@ -208,4 +209,13 @@ public T setOptional(boolean optional) {
this.optional = optional;
return (T) this;
}

public String getSeparator() {
return separator;
}

public T setSeparator(String separator) {
this.separator = separator;
return (T) this;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ protected ServerEndpointIndexer(AbstractBuilder builder) {
this.converterSupplierIndexerExtension = builder.converterSupplierIndexerExtension;
}

@Override
protected void addWriterForType(AdditionalWriters additionalWriters, Type paramType) {
DotName dotName = paramType.name();
if (dotName.equals(JSONP_JSON_VALUE)
Expand All @@ -130,6 +131,7 @@ protected void addWriterForType(AdditionalWriters additionalWriters, Type paramT
}
}

@Override
protected void addReaderForType(AdditionalReaders additionalReaders, Type paramType) {
DotName dotName = paramType.name();
if (dotName.equals(JSONP_JSON_NUMBER)
Expand Down Expand Up @@ -198,6 +200,7 @@ protected boolean handleBeanParam(ClassInfo actualEndpointInfo, Type paramType,
return injectableBean.isFormParamRequired();
}

@Override
protected boolean doesMethodHaveBlockingSignature(MethodInfo info) {
for (var i : methodScanners) {
if (i.isMethodSignatureAsync(info)) {
Expand Down Expand Up @@ -344,7 +347,6 @@ protected MethodParameter createMethodParameter(ClassInfo currentClassInfo, Clas
ParameterConverterSupplier converter = parameterResult.getConverter();
DeclaredTypes declaredTypes = getDeclaredTypes(paramType, currentClassInfo, actualEndpointInfo);
String mimeType = getPartMime(parameterResult.getAnns());
String separator = getSeparator(parameterResult.getAnns());
String declaredType = declaredTypes.getDeclaredType();

if (SUPPORTED_MULTIPART_FILE_TYPES.contains(DotName.createSimple(declaredType))) {
Expand All @@ -354,9 +356,10 @@ protected MethodParameter createMethodParameter(ClassInfo currentClassInfo, Clas
elementType, declaredType, declaredTypes.getDeclaredUnresolvedType(),
type, single, signature,
converter, defaultValue, parameterResult.isObtainedAsCollection(), parameterResult.isOptional(), encoded,
parameterResult.getCustomParameterExtractor(), mimeType, separator);
parameterResult.getCustomParameterExtractor(), mimeType, parameterResult.getSeparator());
}

@Override
protected void handleOtherParam(Map<String, String> existingConverters, String errorLocation, boolean hasRuntimeConverters,
ServerIndexedParameter builder, String elementType) {
try {
Expand All @@ -368,13 +371,15 @@ protected void handleOtherParam(Map<String, String> existingConverters, String e
}
}

@Override
protected void handleSortedSetParam(Map<String, String> existingConverters, String errorLocation,
boolean hasRuntimeConverters, ServerIndexedParameter builder, String elementType) {
ParameterConverterSupplier converter = extractConverter(elementType, index,
existingConverters, errorLocation, hasRuntimeConverters, builder.getAnns());
builder.setConverter(new SortedSetConverter.SortedSetSupplier(converter));
}

@Override
protected void handleOptionalParam(Map<String, String> existingConverters,
Map<DotName, AnnotationInstance> parameterAnnotations,
String errorLocation,
Expand Down Expand Up @@ -409,27 +414,31 @@ protected void handleOptionalParam(Map<String, String> existingConverters,
builder.setConverter(new OptionalConverter.OptionalSupplier(converter));
}

@Override
protected void handleSetParam(Map<String, String> existingConverters, String errorLocation, boolean hasRuntimeConverters,
ServerIndexedParameter builder, String elementType) {
ParameterConverterSupplier converter = extractConverter(elementType, index,
existingConverters, errorLocation, hasRuntimeConverters, builder.getAnns());
builder.setConverter(new SetConverter.SetSupplier(converter));
}

@Override
protected void handleListParam(Map<String, String> existingConverters, String errorLocation, boolean hasRuntimeConverters,
ServerIndexedParameter builder, String elementType) {
ParameterConverterSupplier converter = extractConverter(elementType, index,
existingConverters, errorLocation, hasRuntimeConverters, builder.getAnns());
builder.setConverter(new ListConverter.ListSupplier(converter));
}

@Override
protected void handleArrayParam(Map<String, String> existingConverters, String errorLocation, boolean hasRuntimeConverters,
ServerIndexedParameter builder, String elementType) {
ParameterConverterSupplier converter = extractConverter(elementType, index,
existingConverters, errorLocation, hasRuntimeConverters, builder.getAnns());
builder.setConverter(new ArrayConverter.ArraySupplier(converter, elementType));
}

@Override
protected void handlePathSegmentParam(ServerIndexedParameter builder) {
builder.setConverter(new PathSegmentParamConverter.Supplier());
}
Expand All @@ -439,6 +448,7 @@ protected String handleTrailingSlash(String path) {
return path.substring(0, path.length() - 1);
}

@Override
protected void handleTemporalParam(ServerIndexedParameter builder, DotName paramType,
Map<DotName, AnnotationInstance> parameterAnnotations,
MethodInfo currentMethodInfo) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -275,26 +275,27 @@ public void visitEnd() {
break;
case FORM:
injectParameterWithConverter(injectMethod, "getFormParameter", fieldInfo, extractor, true, true,
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED));
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED), false);
break;
case HEADER:
injectParameterWithConverter(injectMethod, "getHeader", fieldInfo, extractor, true, false, false);
injectParameterWithConverter(injectMethod, "getHeader", fieldInfo, extractor, true, false, false,
false);
break;
case MATRIX:
injectParameterWithConverter(injectMethod, "getMatrixParameter", fieldInfo, extractor, true, true,
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED));
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED), false);
break;
case COOKIE:
injectParameterWithConverter(injectMethod, "getCookieParameter", fieldInfo, extractor, false, false,
false);
false, false);
break;
case PATH:
injectParameterWithConverter(injectMethod, "getPathParameter", fieldInfo, extractor, false, true,
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED));
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED), false);
break;
case QUERY:
injectParameterWithConverter(injectMethod, "getQueryParameter", fieldInfo, extractor, true, true,
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED));
fieldInfo.hasAnnotation(ResteasyReactiveDotNames.ENCODED), true);
break;
default:
break;
Expand Down Expand Up @@ -518,7 +519,8 @@ private ParameterConverterSupplier removeRuntimeResolvedConverterDelegate(Parame
}

private void injectParameterWithConverter(MethodVisitor injectMethod, String methodName, FieldInfo fieldInfo,
ServerIndexedParameter extractor, boolean extraSingleParameter, boolean extraEncodedParam, boolean encoded) {
ServerIndexedParameter extractor, boolean extraSingleParameter, boolean extraEncodedParam, boolean encoded,
boolean extraSeparatorParam) {

// spec says:
/*
Expand Down Expand Up @@ -552,7 +554,8 @@ private void injectParameterWithConverter(MethodVisitor injectMethod, String met
// push the parameter value
MultipartFormParamExtractor.Type multipartType = getMultipartFormType(extractor);
if (multipartType == null) {
loadParameter(injectMethod, methodName, extractor, extraSingleParameter, extraEncodedParam, encoded);
loadParameter(injectMethod, methodName, extractor, extraSingleParameter, extraEncodedParam, encoded,
extraSeparatorParam);
} else {
loadMultipartParameter(injectMethod, fieldInfo, extractor, multipartType);
}
Expand Down Expand Up @@ -886,13 +889,22 @@ private void convertParameter(MethodVisitor injectMethod, ServerIndexedParameter
}

private void loadParameter(MethodVisitor injectMethod, String methodName, IndexedParameter extractor,
boolean extraSingleParameter, boolean extraEncodedParam, boolean encoded) {
boolean extraSingleParameter, boolean extraEncodedParam, boolean encoded, boolean extraSeparatorParam) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need the "extraSeparatorParam" flag? Wouldn't be the same to use "extractor.getSeparator() != null" instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The loadParameter method generates bytecode to call the method whose name is methodName. For this, we need to put the correct arguments on the stack. If we want to call the getQueryParameter method (in its modified form), then we need to put the four parameters name, single, encoded and separator on the stack. If the separator is null, then we should push null on the stack. Hence, it is possible that extraSeparatorParam is true but extractor.getSeparator() is null.

// ctx param
injectMethod.visitIntInsn(Opcodes.ALOAD, 1);
// name param
injectMethod.visitLdcInsn(extractor.getName());
String methodSignature;
if (extraEncodedParam && extraSingleParameter) {
if (extraEncodedParam && extraSingleParameter && extraSeparatorParam) {
injectMethod.visitLdcInsn(extractor.isSingle());
injectMethod.visitLdcInsn(encoded);
if (extractor.getSeparator() != null) {
injectMethod.visitLdcInsn(extractor.getSeparator());
} else {
injectMethod.visitInsn(Opcodes.ACONST_NULL);
}
methodSignature = "(Ljava/lang/String;ZZLjava/lang/String;)Ljava/lang/Object;";
} else if (extraEncodedParam && extraSingleParameter) {
injectMethod.visitLdcInsn(extractor.isSingle());
injectMethod.visitLdcInsn(encoded);
methodSignature = "(Ljava/lang/String;ZZ)Ljava/lang/Object;";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public ResteasyReactiveRequestContext(Deployment deployment,

public abstract ServerHttpRequest serverRequest();

@Override
public abstract ServerHttpResponse serverResponse();

public Deployment getDeployment() {
Expand Down Expand Up @@ -286,6 +287,7 @@ public Object getResult() {
return result;
}

@Override
public Throwable getThrowable() {
return throwable;
}
Expand Down Expand Up @@ -618,6 +620,7 @@ public ResteasyReactiveRequestContext setWriterInterceptors(WriterInterceptor[]
return this;
}

@Override
protected void handleUnrecoverableError(Throwable throwable) {
log.error("Request failed", throwable);
endResponse();
Expand All @@ -632,6 +635,7 @@ protected void endResponse() {
close();
}

@Override
protected void handleRequestScopeActivation() {
CurrentRequestManager.set(this);
}
Expand Down Expand Up @@ -707,6 +711,7 @@ public boolean hasInputStream() {
return inputStream != null;
}

@Override
public InputStream getInputStream() {
if (inputStream == null) {
inputStream = serverRequest().createInputStream();
Expand Down Expand Up @@ -809,25 +814,40 @@ public Object getHeader(String name, boolean single) {
}
}

@Override
public Object getQueryParameter(String name, boolean single, boolean encoded) {
return getQueryParameter(name, single, encoded, null);
}

@Override
public Object getQueryParameter(String name, boolean single, boolean encoded, String separator) {
if (single) {
String val = serverRequest().getQueryParam(name);
if (encoded && val != null) {
val = Encode.encodeQueryParam(val);
}
return val;
}

// empty collections must not be turned to null
List<String> strings = serverRequest().getAllQueryParams(name);
if (encoded) {
List<String> newStrings = new ArrayList<>();
for (String i : strings) {
newStrings.add(Encode.encodeQueryParam(i));
}
return newStrings;
strings = newStrings;
}

if (separator != null) {
List<String> result = new ArrayList<>(strings.size());
for (int i = 0; i < strings.size(); i++) {
String[] parts = strings.get(i).split(separator);
result.addAll(Arrays.asList(parts));
}
return result;
} else {
return strings;
}
return strings;
}

@Override
Expand Down Expand Up @@ -920,6 +940,7 @@ public String getPathParameter(String name, boolean encoded) {
return value;
}

@Override
public <T> T unwrap(Class<T> type) {
return serverRequest().unwrap(type);
}
Expand Down Expand Up @@ -957,6 +978,7 @@ public OutputStream getOutputStream() {
return outputStream;
}

@Override
public OutputStream getOrCreateOutputStream() {
if (outputStream == null) {
return outputStream = underlyingOutputStream = serverResponse().createResponseOutputStream();
Expand Down
Loading