search

linkedin / parseq

Asynchronous Java made easier
Apache License 2.0
1.17k stars 266 forks source link

Adding cookies to the generated request from GetRequestGroup #280

Closed logancarmody closed 4 years ago

logancarmody commented 4 years ago

Currently, the GetRequestGroup implementation leaves out cookies while batching requests. This causes issues, especially for batched requests to frontends which may use cookies for authentication logic.

Likely the assumption was the cookies were included in the headers, but unfortunately they are not in the R2 Request object.

This change might be backwards incompatible for some requests. It will not change the ability for requests to be batched, because currently the key in the map uses the whole request object to check for equality, so it is already checking for cookie equality. However we aren't currently sending the cookies with the batch-get request, so if that could transform the response, then this would be backwards incompatible.

BrianPin commented 4 years ago

Hi Logan,

Would it be safe to add a configuration for this feature?

By default the feature of this change can be disabled.

Only the cases you mentioned can turn on it.

logancarmody commented 4 years ago

Sure would be happy to add a flag.

logancarmody commented 4 years ago

On second thought, I think this is a bug and introducing a boolean flag here is very semantically weird.

We would have to surface it on the ParseqRestClient level, which would be a flag which is _shouldAddCookiesToBatchedRequests.

Additionally, cookies can impact behavior server-side and are already being used to determine whether to batch the requests, so it would be incorrect to expose a configuration to not attach them