ok2c commented on code in PR #741: URL: https://github.com/apache/httpcomponents-client/pull/741#discussion_r2440080005
########## httpclient5/src/main/java/org/apache/hc/client5/http/protocol/H2RequestPriority.java: ########## @@ -0,0 +1,121 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.http.protocol; + +import java.io.IOException; + +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolVersion; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.hc.core5.http2.priority.PriorityFormatter; +import org.apache.hc.core5.http2.priority.PriorityValue; +import org.apache.hc.core5.util.Args; + +/** + * Emits {@code Priority} request header for HTTP/2+. + * <p> + * The priority value is taken from the request context attribute + * {@link #ATTR_HTTP2_PRIORITY_VALUE}. If the formatted value equals + * RFC defaults (u=3, i=false) the header is omitted. + * <p> + * If {@code overwrite} is {@code false} (default), an existing {@code Priority} + * header set by the caller is preserved. + * + * @since 5.6 + */ +@Experimental +@Contract(threading = ThreadingBehavior.IMMUTABLE) +public final class H2RequestPriority implements HttpRequestInterceptor { + + /** + * Context attribute to carry a {@link PriorityValue}. + */ + public static final String ATTR_HTTP2_PRIORITY_VALUE = "http2.priority.value"; Review Comment: @arturobernalg We should not do that. Either introduce a proper attribute in the `HttpClientContext` or better yet add it to the `RequestConfig` which is the proper place for request level parameters. ########## httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/H2AsyncClientBuilder.java: ########## @@ -699,6 +700,27 @@ public final H2AsyncClientBuilder evictIdleConnections(final TimeValue maxIdleTi return this; } + /** + * Enables emission of the RFC 9218 {@code Priority} request header for HTTP/2 requests. Review Comment: @arturobernalg Making direct references to specific RFCs or even copying bits of those specifications to our source code did us little good in the past. We should avoid that in all new features. RFC 9218 can be superseded or obsoleted by another RFC in the future. ########## httpclient5/src/test/java/org/apache/hc/client5/http/examples/ClientH2PriorityHeaderExample.java: ########## @@ -0,0 +1,75 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ +package org.apache.hc.client5.http.examples; + +import java.util.concurrent.Future; + +import org.apache.hc.client5.http.async.methods.SimpleHttpRequest; +import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; +import org.apache.hc.client5.http.async.methods.SimpleRequestBuilder; +import org.apache.hc.client5.http.impl.async.CloseableHttpAsyncClient; +import org.apache.hc.client5.http.impl.async.H2AsyncClientBuilder; +import org.apache.hc.client5.http.protocol.H2RequestPriority; +import org.apache.hc.client5.http.protocol.HttpClientContext; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.http2.config.H2Config; +import org.apache.hc.core5.http2.priority.PriorityValue; + +@Experimental +public class ClientH2PriorityHeaderExample { Review Comment: @arturobernalg Could you please make the class name more consistent with the rest of examples? ########## httpclient5/src/main/java/org/apache/hc/client5/http/protocol/H2RequestPriority.java: ########## @@ -0,0 +1,121 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.http.protocol; + +import java.io.IOException; + +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolVersion; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.hc.core5.http2.priority.PriorityFormatter; +import org.apache.hc.core5.http2.priority.PriorityValue; +import org.apache.hc.core5.util.Args; + +/** + * Emits {@code Priority} request header for HTTP/2+. + * <p> + * The priority value is taken from the request context attribute + * {@link #ATTR_HTTP2_PRIORITY_VALUE}. If the formatted value equals + * RFC defaults (u=3, i=false) the header is omitted. + * <p> + * If {@code overwrite} is {@code false} (default), an existing {@code Priority} + * header set by the caller is preserved. + * + * @since 5.6 + */ +@Experimental +@Contract(threading = ThreadingBehavior.IMMUTABLE) +public final class H2RequestPriority implements HttpRequestInterceptor { + + /** + * Context attribute to carry a {@link PriorityValue}. + */ + public static final String ATTR_HTTP2_PRIORITY_VALUE = "http2.priority.value"; + + /** + * Singleton with {@code overwrite=false}. + */ + public static final H2RequestPriority INSTANCE = new H2RequestPriority(false); + + private final boolean overwrite; + + public H2RequestPriority() { + this(false); + } + + public H2RequestPriority(final boolean overwrite) { + this.overwrite = overwrite; + } + + @Override + public void process(final HttpRequest request, final EntityDetails entity, + final HttpContext context) throws HttpException, IOException { + + Args.notNull(request, "HTTP request"); + Args.notNull(context, "HTTP context"); + + final ProtocolVersion ver = HttpCoreContext.cast(context).getProtocolVersion(); + if (ver == null || ver.compareToVersion(HttpVersion.HTTP_2) < 0) { + return; // only for HTTP/2+ + } + + final Header existing = request.getFirstHeader(HttpHeaders.PRIORITY); + if (existing != null && !overwrite) { Review Comment: @arturobernalg This overwrite thing makes the whole method terribly messy, ########## httpclient5/src/main/java/org/apache/hc/client5/http/protocol/H2RequestPriority.java: ########## @@ -0,0 +1,121 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.http.protocol; + +import java.io.IOException; + +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolVersion; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.hc.core5.http2.priority.PriorityFormatter; +import org.apache.hc.core5.http2.priority.PriorityValue; +import org.apache.hc.core5.util.Args; + +/** + * Emits {@code Priority} request header for HTTP/2+. + * <p> + * The priority value is taken from the request context attribute + * {@link #ATTR_HTTP2_PRIORITY_VALUE}. If the formatted value equals + * RFC defaults (u=3, i=false) the header is omitted. + * <p> + * If {@code overwrite} is {@code false} (default), an existing {@code Priority} + * header set by the caller is preserved. + * + * @since 5.6 + */ +@Experimental +@Contract(threading = ThreadingBehavior.IMMUTABLE) +public final class H2RequestPriority implements HttpRequestInterceptor { + + /** + * Context attribute to carry a {@link PriorityValue}. + */ + public static final String ATTR_HTTP2_PRIORITY_VALUE = "http2.priority.value"; + + /** + * Singleton with {@code overwrite=false}. + */ + public static final H2RequestPriority INSTANCE = new H2RequestPriority(false); + + private final boolean overwrite; + + public H2RequestPriority() { + this(false); + } + + public H2RequestPriority(final boolean overwrite) { + this.overwrite = overwrite; + } + + @Override + public void process(final HttpRequest request, final EntityDetails entity, + final HttpContext context) throws HttpException, IOException { + + Args.notNull(request, "HTTP request"); + Args.notNull(context, "HTTP context"); + + final ProtocolVersion ver = HttpCoreContext.cast(context).getProtocolVersion(); Review Comment: @arturobernalg Oh, please do not do that. Please do not call `HttpCoreContext.cast(context)` multiple times in the same method. It is cheap and not that cheap.. ########## httpclient5/src/main/java/org/apache/hc/client5/http/protocol/H2RequestPriority.java: ########## @@ -0,0 +1,121 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.http.protocol; + +import java.io.IOException; + +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolVersion; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.hc.core5.http2.priority.PriorityFormatter; +import org.apache.hc.core5.http2.priority.PriorityValue; +import org.apache.hc.core5.util.Args; + +/** + * Emits {@code Priority} request header for HTTP/2+. + * <p> + * The priority value is taken from the request context attribute + * {@link #ATTR_HTTP2_PRIORITY_VALUE}. If the formatted value equals + * RFC defaults (u=3, i=false) the header is omitted. + * <p> + * If {@code overwrite} is {@code false} (default), an existing {@code Priority} + * header set by the caller is preserved. + * + * @since 5.6 + */ +@Experimental +@Contract(threading = ThreadingBehavior.IMMUTABLE) +public final class H2RequestPriority implements HttpRequestInterceptor { + + /** + * Context attribute to carry a {@link PriorityValue}. + */ + public static final String ATTR_HTTP2_PRIORITY_VALUE = "http2.priority.value"; + + /** + * Singleton with {@code overwrite=false}. + */ + public static final H2RequestPriority INSTANCE = new H2RequestPriority(false); + + private final boolean overwrite; Review Comment: @arturobernalg Why this overwrite? HttpClient does not usually overwrite headers explicitly set by the user. ########## httpclient5/src/main/java/org/apache/hc/client5/http/protocol/H2RequestPriority.java: ########## @@ -0,0 +1,121 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * <http://www.apache.org/>. + * + */ + +package org.apache.hc.client5.http.protocol; + +import java.io.IOException; + +import org.apache.hc.core5.annotation.Contract; +import org.apache.hc.core5.annotation.Experimental; +import org.apache.hc.core5.annotation.ThreadingBehavior; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.Header; +import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpRequestInterceptor; +import org.apache.hc.core5.http.HttpVersion; +import org.apache.hc.core5.http.ProtocolVersion; +import org.apache.hc.core5.http.protocol.HttpContext; +import org.apache.hc.core5.http.protocol.HttpCoreContext; +import org.apache.hc.core5.http2.priority.PriorityFormatter; +import org.apache.hc.core5.http2.priority.PriorityValue; +import org.apache.hc.core5.util.Args; + +/** + * Emits {@code Priority} request header for HTTP/2+. + * <p> + * The priority value is taken from the request context attribute + * {@link #ATTR_HTTP2_PRIORITY_VALUE}. If the formatted value equals + * RFC defaults (u=3, i=false) the header is omitted. + * <p> + * If {@code overwrite} is {@code false} (default), an existing {@code Priority} + * header set by the caller is preserved. + * + * @since 5.6 + */ +@Experimental +@Contract(threading = ThreadingBehavior.IMMUTABLE) +public final class H2RequestPriority implements HttpRequestInterceptor { + + /** + * Context attribute to carry a {@link PriorityValue}. + */ + public static final String ATTR_HTTP2_PRIORITY_VALUE = "http2.priority.value"; + + /** + * Singleton with {@code overwrite=false}. + */ + public static final H2RequestPriority INSTANCE = new H2RequestPriority(false); + + private final boolean overwrite; + + public H2RequestPriority() { + this(false); + } + + public H2RequestPriority(final boolean overwrite) { + this.overwrite = overwrite; + } + + @Override + public void process(final HttpRequest request, final EntityDetails entity, + final HttpContext context) throws HttpException, IOException { + + Args.notNull(request, "HTTP request"); + Args.notNull(context, "HTTP context"); + + final ProtocolVersion ver = HttpCoreContext.cast(context).getProtocolVersion(); + if (ver == null || ver.compareToVersion(HttpVersion.HTTP_2) < 0) { Review Comment: @arturobernalg `HttpVersion` returned by this method is never null. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
