async-http-client icon indicating copy to clipboard operation
async-http-client copied to clipboard

Provide an ImmutableRequestBuilder so addQueryParams/setQueryParams method argument are not mutated

Open lpfeup opened this issue 7 years ago • 4 comments

the addQueryParams/setQueryParams methods are assigning the passed argument directly to the class field queryParams. This has unwanted side-effects.

Code:

RequestBuilderBase.addQueryParams

RequestBuilderBase.setQueryParams

Example:

List<Param> params = new ArrayList<>();
params.add(new Param("testKey", "testVal"));
		
Request request = httpClient.prepareGet(url)
                           .setQueryParams(params)
                           .addQueryParam("testKey2", "testVal2") // params list is modified here
                           .build();

params.forEach(p -> System.out.println(p.getName() + ": " + p.getValue()));
// EXPECTED:
// testKey: testVal

// OUTPUT:
// testKey: testVal
// testKey2: testVal2

Another example:

// params as unmodifiable collection
List<Param> params = Collections.singletonList(new Param("testKey", "testVal"));
		
Request request = httpClient.prepareGet(url)
                           .setQueryParams(params)
                           .addQueryParam("testKey2", "testVal2")
                           .build();

// throws java.lang.UnsupportedOperationException on addQueryParam("testKey2", "testVal2")

I suggest replacing the assignments queryParams = params with copies as below:

queryParams = new ArrayList<>(params);

Would you be willing to accept a pull request?

lpfeup avatar May 03 '18 10:05 lpfeup

The current behavior is intended: mutable builder for best performance with limited allocations.

Would you be willing to accept a pull request?

Maybe introduce a second ImmutableRequestBuilder?

slandelle avatar May 03 '18 10:05 slandelle

Just for clarification, ImmutableRequestBuilder would still be mutating it's internal state on each method call. But it would no longer modify objects passed as arguments from calling methods.

Is that it?

lpfeup avatar May 03 '18 11:05 lpfeup

No, please go with full immutability.

slandelle avatar May 03 '18 11:05 slandelle

Similar to Scala case classes.

slandelle avatar May 03 '18 11:05 slandelle