Skip to content

Enable use of nullable maps for headers, params and query #694

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

Open
kamperiadis wants to merge 6 commits into
base: dev
Choose a base branch
from
Open

Enable use of nullable maps for headers, params and query #694

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea4df5f
Enable use of nullable maps for headers, params and query
kamperiadis Jan 20, 2023
5c74367
Check app setting and filter map
kamperiadis Jan 23, 2023
1e53683
Simplify unit tests
kamperiadis Jan 25, 2023
83d5388
Add app setting name in constants file
kamperiadis Jan 27, 2023
60bb55a
Merge branch 'dev' into kamperiadis/nullableMap
kamperiadis Jan 27, 2023
727323c
Merge branch 'dev' into kamperiadis/nullableMap
kamperiadis Jan 31, 2023
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
25 changes: 21 additions & 4 deletions src/main/java/com/microsoft/azure/functions/worker/binding/RpcHttpRequestDataSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,10 @@
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

import com.microsoft.azure.functions.rpc.messages.NullableTypes;
import com.microsoft.azure.functions.worker.Util;
import org.apache.commons.lang3.reflect.TypeUtils;

import com.microsoft.azure.functions.HttpMethod;
Expand All @@ -22,8 +25,10 @@ public RpcHttpRequestDataSource(String name, RpcHttp value) {
super(name, null, HTTP_DATA_OPERATIONS);
this.httpPayload = value;
this.bodyDataSource = BindingDataStore.rpcSourceFromTypedData(null, this.httpPayload.getBody());
this.fields = Arrays.asList(this.httpPayload.getHeadersMap(), this.httpPayload.getQueryMap(),
this.httpPayload.getParamsMap());
this.fields = Arrays.asList(
convertFromNullableMap(this.httpPayload.getNullableHeadersMap()),
convertFromNullableMap(this.httpPayload.getNullableQueryMap()),
convertFromNullableMap(this.httpPayload.getNullableParamsMap()));
Copy link
Member

@kaibocai kaibocai Jan 23, 2023

Choose a reason for hiding this comment

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

not related to this PR, but do we know difference between query map and param map?

Copy link
Collaborator Author

@kamperiadis kamperiadis Jan 25, 2023

Choose a reason for hiding this comment

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

Looking at the host code, it looks like the three key differences between query map and param map are that:

  1. They come from different parts of the request object (i.e. request.Query vs request.HttpContext.Items.TryGetValue(HttpExtensionConstants.AzureWebJobsHttpRouteDataKey, out object routeData))
  2. We do not add null values to the parameters map, even if the UseNullableValueDictionaryForHttp capability is on
  3. We add empty strings to the parameters map, even if the UseNullableValueDictionaryForHttp capability is off

this.setValue(this);
}

Expand All @@ -45,12 +50,12 @@ public HttpMethod getHttpMethod() {

@Override
public Map<String, String> getHeaders() {
return this.parentDataSource.httpPayload.getHeadersMap();
return convertFromNullableMap(this.parentDataSource.httpPayload.getNullableHeadersMap());
}

@Override
public Map<String, String> getQueryParameters() {
return this.parentDataSource.httpPayload.getQueryMap();
return convertFromNullableMap(this.parentDataSource.httpPayload.getNullableQueryMap());
}

@Override
Expand Down Expand Up @@ -86,4 +91,16 @@ public Builder createResponseBuilder(HttpStatus status) {
return new HttpRequestMessageImpl(v, bodyData.getValue());
});
}

private static Map<String, String> convertFromNullableMap(Map<String, NullableTypes.NullableString> nullableMap) {
if (Util.isTrue(System.getenv("FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED"))) {
Copy link
Member

@shreyas-gopalakrishna shreyas-gopalakrishna Jan 26, 2023

Choose a reason for hiding this comment

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

Let us make FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED a constant.

Copy link
Member

@kaibocai kaibocai Jan 26, 2023

Choose a reason for hiding this comment

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

nice catch! there is a constant file you can define it there.

return nullableMap.entrySet().stream().collect(
Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue())
);
} else {
return nullableMap.entrySet().stream()
.filter(e -> e.getValue().getValue() != null && !e.getValue().getValue().trim().isEmpty())
.collect(Collectors.toMap(Map.Entry::getKey, e -> e.getValue().getValue()));
}
}
}
1 change: 1 addition & 0 deletions src/main/java/com/microsoft/azure/functions/worker/handler/WorkerInitRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ String execute(WorkerInitRequest request, WorkerInitResponse.Builder response) {
response.putCapabilities("TypedDataCollection", "TypedDataCollection");
response.putCapabilities("WorkerStatus", "WorkerStatus");
response.putCapabilities("RpcHttpBodyOnly", "RpcHttpBodyOnly");
response.putCapabilities("UseNullableValueDictionaryForHttp", "UseNullableValueDictionaryForHttp");
response.putCapabilities("RpcHttpTriggerMetadataRemoved", "RpcHttpTriggerMetadataRemoved");
response.putCapabilities("HandlesWorkerTerminateMessage", "HandlesWorkerTerminateMessage");
response.setWorkerMetadata(composeWorkerMetadata());
Expand Down
165 changes: 158 additions & 7 deletions ...java/com/microsoft/azure/functions/worker/binding/tests/RpcHttpRequestDataSourceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,16 @@
import java.lang.reflect.Method;
import java.lang.reflect.Parameter;
import java.util.*;

import com.microsoft.azure.functions.rpc.messages.NullableTypes.NullableString;
import com.microsoft.azure.functions.HttpRequestMessage;
import com.microsoft.azure.functions.HttpStatus;
import com.microsoft.azure.functions.rpc.messages.RpcHttp;
import com.microsoft.azure.functions.rpc.messages.TypedData;
import com.microsoft.azure.functions.worker.binding.*;
import com.microsoft.azure.functions.worker.broker.JavaFunctionBroker;
import com.microsoft.azure.functions.worker.handler.FunctionEnvironmentReloadRequestHandler;
import com.microsoft.azure.functions.worker.reflect.DefaultClassLoaderProvider;
import org.junit.jupiter.api.Test;

import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -26,13 +31,23 @@ public void HttpRequestIntBody(HttpRequestMessage<Integer> request) {
public void HttpRequestBinaryBody(HttpRequestMessage<byte[]> request) {
}

public static RpcHttp getTestRpcHttp(Object inputBody) throws Exception {
public static RpcHttp getTestRpcHttp(
Object inputBody,
Map<String, String> headersMap,
Map<String, String> queryMap) throws Exception {
TypedData.Builder dataBuilder = TypedData.newBuilder();
RpcHttp.Builder httpBuilder = RpcHttp.newBuilder()
.setStatusCode(Integer.toString(HttpStatus.OK.value()));
Map<String, String> headers = new HashMap<>();
headers.put("header", "testHeader");
headers.forEach(httpBuilder::putHeaders);
if (headersMap != null) {
for (String key : headersMap.keySet()) {
httpBuilder.putNullableHeaders(key, NullableString.newBuilder().setValue(headersMap.get(key)).build());
}
}
if (queryMap != null) {
for (String key : queryMap.keySet()) {
httpBuilder.putNullableQuery(key, NullableString.newBuilder().setValue(queryMap.get(key)).build());
}
}
RpcUnspecifiedDataTarget bodyTarget = new RpcUnspecifiedDataTarget();
Object body = inputBody;
bodyTarget.setValue(body);
Expand All @@ -41,14 +56,150 @@ public static RpcHttp getTestRpcHttp(Object inputBody) throws Exception {
return httpBuilder.build();
}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableQueryParamsEmpty_EnvSettingEnabled() throws Exception {
Copy link
Member

@kaibocai kaibocai Jan 23, 2023

Choose a reason for hiding this comment

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

let's use camel-case syntax for method name, the old tests were wrote by c# expert, seems they are using c# name convention.

DefaultClassLoaderProvider classLoader = new DefaultClassLoaderProvider();
JavaFunctionBroker broker = new JavaFunctionBroker(classLoader);
FunctionEnvironmentReloadRequestHandler envHandler = new FunctionEnvironmentReloadRequestHandler(broker);
Map<String, String> existingVariables = System.getenv();
Map<String, String> newEnvVariables = new HashMap<>();
newEnvVariables.putAll(existingVariables);
newEnvVariables.put("FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED", "true");
envHandler.setEnv(newEnvVariables);
Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> queryMap = new HashMap<String, String>() {{
put("name", "");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, null, queryMap);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getQueryParameters().get("name"), "");
Copy link
Member

@kaibocai kaibocai Jan 23, 2023

Choose a reason for hiding this comment

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

let's follow the convention that expect value comes first and test value comes second. I think we can use assertEmpty here.

}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableQueryParamsEmpty_EnvSettingDisabled() throws Exception {
DefaultClassLoaderProvider classLoader = new DefaultClassLoaderProvider();
JavaFunctionBroker broker = new JavaFunctionBroker(classLoader);
FunctionEnvironmentReloadRequestHandler envHandler = new FunctionEnvironmentReloadRequestHandler(broker);
Map<String, String> existingVariables = System.getenv();
Map<String, String> newEnvVariables = new HashMap<>();
newEnvVariables.putAll(existingVariables);
newEnvVariables.put("FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED", "false");
envHandler.setEnv(newEnvVariables);
Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> queryMap = new HashMap<String, String>() {{
put("name", "");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, null, queryMap);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getQueryParameters().get("name"), null);
Copy link
Member

@kaibocai kaibocai Jan 23, 2023

Choose a reason for hiding this comment

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

we can use assertNull

}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableQueryParamsNonEmpty() throws Exception {
Copy link
Member

@kaibocai kaibocai Jan 23, 2023

Choose a reason for hiding this comment

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

I think we can have the test cases more clear. There are two major scenarios we want to test

  1. appsetting to false
  2. appsettting to true

each of those scenarios we want to test

  1. normal case -- param value empty
  2. param value empty

So we can have two test cases in total

  1. with appsetting to true
  • have one normal param -- expect same value
  • have one param with empty value -- expect empty string
  1. with appsetting to false
  • have one normal param -- expect normal value
  • have one param with empty value -- expect null

We can test header map and query map in same test method.
So we can reduce the test methods number to 2 but still have full coverage. Let me know if this make sense or not. Thanks.


Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> queryMap = new HashMap<String, String>() {{
put("name", "random");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, null, queryMap);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getQueryParameters().get("name"), "random");
}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableHeadersEmpty_EnvSettingEnabled() throws Exception {
DefaultClassLoaderProvider classLoader = new DefaultClassLoaderProvider();
JavaFunctionBroker broker = new JavaFunctionBroker(classLoader);
FunctionEnvironmentReloadRequestHandler envHandler = new FunctionEnvironmentReloadRequestHandler(broker);
Map<String, String> existingVariables = System.getenv();
Map<String, String> newEnvVariables = new HashMap<>();
newEnvVariables.putAll(existingVariables);
newEnvVariables.put("FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED", "true");
envHandler.setEnv(newEnvVariables);
Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> headersMap = new HashMap<String, String>() {{
put("cookie", "");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, headersMap, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getHeaders().get("cookie"), "");
}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableHeadersEmpty_EnvSettingDisabled() throws Exception {
DefaultClassLoaderProvider classLoader = new DefaultClassLoaderProvider();
JavaFunctionBroker broker = new JavaFunctionBroker(classLoader);
FunctionEnvironmentReloadRequestHandler envHandler = new FunctionEnvironmentReloadRequestHandler(broker);
Map<String, String> existingVariables = System.getenv();
Map<String, String> newEnvVariables = new HashMap<>();
newEnvVariables.putAll(existingVariables);
newEnvVariables.put("FUNCTIONS_WORKER_NULLABLE_VALUES_ENABLED", "false");
envHandler.setEnv(newEnvVariables);
Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> headersMap = new HashMap<String, String>() {{
put("cookie", "");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, headersMap, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getHeaders().get("cookie"), null);
}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_NullableHeadersNonEmpty() throws Exception {

Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");
Map<String, String> headersMap = new HashMap<String, String>() {{
put("cookie", "foo=bar");
}};
Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(null, headersMap, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
BindingData actualArg = actualBindingData.orElseThrow(WrongMethodTypeException::new);
HttpRequestMessage<?> requestMsg = (HttpRequestMessage<?>) actualArg.getValue();
assertEquals(requestMsg.getHeaders().get("cookie"), "foo=bar");
}

@Test
public void rpcHttpDataSource_To_HttpRequestMessage_StringBody() throws Exception {

Method httpRequestMessageStringBodyMethod = getFunctionMethod("HttpRequestStringBody");

Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp("testStringBody");
RpcHttp input = getTestRpcHttp("testStringBody", null, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
Expand All @@ -64,7 +215,7 @@ public void rpcHttpDataSource_To_HttpRequestMessage_IntegerBody() throws Excepti

Parameter[] parameters = httpRequestMessageStringBodyMethod.getParameters();
String sourceKey = "testRpcHttp";
RpcHttp input = getTestRpcHttp(1234);
RpcHttp input = getTestRpcHttp(1234, null, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
Expand All @@ -82,7 +233,7 @@ public void rpcHttpDataSource_To_HttpRequestMessage_byteArrayBody() throws Excep
String sourceKey = "testRpcHttp";
String expectedString = "Example String";
byte[] inputBytes = expectedString.getBytes();
RpcHttp input = getTestRpcHttp(inputBytes);
RpcHttp input = getTestRpcHttp(inputBytes, null, null);
RpcHttpRequestDataSource rpcHttp = new RpcHttpRequestDataSource(sourceKey, input);
Optional<BindingData> actualBindingData = rpcHttp.computeByName(sourceKey,
parameters[0].getParameterizedType());
Expand Down