Add some useful API methods #1295
Conversation
… and set headers method for headers with single values
|
Hi, There's too many different things in this PR. It would be better to isolate them. I'm OK with the first one (once again, I'm so sad/disappointed by Java's type erasure...). See comments. Regarding the second one, I'm not fond of it depending on the Servlet API. I would be OK with isolated bridges that only require an optional dependency, though. |
| import org.slf4j.LoggerFactory; | ||
| import static org.asynchttpclient.util.HttpUtils.parseCharset; | ||
| import static org.asynchttpclient.util.HttpUtils.validateSupportedScheme; | ||
| import static org.asynchttpclient.util.MiscUtils.isNonEmpty; |
There was a problem hiding this comment.
Please revert imports ordering: unrelated.
| import static org.asynchttpclient.cookie.CookieUtil.*; | ||
| import static org.asynchttpclient.cookie.CookieUtil.validateCookieAttribute; | ||
| import static org.asynchttpclient.cookie.CookieUtil.validateCookieName; | ||
| import static org.asynchttpclient.cookie.CookieUtil.validateCookieValue; |
| } | ||
| return asDerivedType(); | ||
| } | ||
|
|
||
| public T setSingleHeaders(Map<String, String> headers) { | ||
| this.headers.clear(); | ||
| headers.forEach((name, value) -> this.headers.add(name, value)); |
There was a problem hiding this comment.
missing null check, like in setHeaders
| } | ||
| return asDerivedType(); | ||
| } | ||
|
|
||
| public T setSingleHeaders(Map<String, String> headers) { | ||
| this.headers.clear(); |
There was a problem hiding this comment.
add a clearHeaders method?
| } | ||
| return asDerivedType(); | ||
| } | ||
|
|
||
| public T setSingleHeaders(Map<String, String> headers) { |
| <groupId>javax.servlet</groupId> | ||
| <artifactId>javax.servlet-api</artifactId> | ||
| <version>${javax.servlet-api.version}</version> | ||
| <optional>true</optional> |
There was a problem hiding this comment.
Are you sure this dep is really optional? I feel like the Cookie class would fail to load and crash.
There was a problem hiding this comment.
No, just import doesn't mean anything. Cookie class would be loaded (and fail) if somebody actually would use these new methods.
|
@slandelle +1 for dropping cookie in favour of Netty's Cookie.
Current code should be absolutely fine with optional dependency (so, if nobody will add javax.servlet and would not use new conversion methods - there would be no ClassDefNotFound), but I would double check. |
|
Closing this. |
Motivation and proposals:
setHeaders(Map<String, Collection<String>> headers)is general, but in most cases applications use headers with only one value, so it's good to have a simple method to set them without convertingMap<String, String>toMap<String, Collections.singletonList<String>>.org.asynchttpclient.Cookiemanually. Expose two helper factory methods set for Netty's and standardjavax.servletentities.These methods were implemented as static utils for async-http-client in my project, I think it would be handy to have them out of the box.