AsyncHttpClient / async-http-client

Asynchronous Http and WebSocket Client library for Java
Other
6.29k stars 1.59k forks source link

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

Open lpfeup opened 6 years ago

lpfeup commented 6 years ago

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?

slandelle commented 6 years ago

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?

lpfeup commented 6 years ago

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?

slandelle commented 6 years ago

No, please go with full immutability.

slandelle commented 6 years ago

Similar to Scala case classes.