Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve Javadocs for styx-api module #271

Merged
merged 13 commits into from
Sep 10, 2018
Merged

Improve Javadocs for styx-api module #271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
85effc1
Add javadoc maven plugin and a package-info.java.
mikkokar Sep 5, 2018
7b8e91e
Add package-level api docs for `com.hotels.styx.api`.
mikkokar Sep 5, 2018
242368c
Add javadoc: ContentOverflowException.
mikkokar Sep 5, 2018
a1f7ddb
Restrict visibility of FullHttpMessage interface.
mikkokar Sep 5, 2018
5c37db7
A class-level JavaDocs for FullHttpRequest and Response objects.
mikkokar Sep 5, 2018
e8145bf
A class-level JavaDocs for HttpRequest and Response objects.
mikkokar Sep 5, 2018
0341513
A JavaDocs for HttpRequest and Response objects.
mikkokar Sep 6, 2018
2077b18
Address code review comments.
mikkokar Sep 6, 2018
7727240
Fix static analysis failures.
mikkokar Sep 6, 2018
1a0f6bc
Improve javadocs for few additional api classes.
mikkokar Sep 6, 2018
c7cdd9b
Address code review comments.
mikkokar Sep 6, 2018
73da384
Address code review comments.
mikkokar Sep 7, 2018
ec881db
Checkstyle: Allow empty javadoc description.
mikkokar Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codequality/checkstyle_rules.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
</module>
<module name="JavadocStyle">
<property name="severity" value="error"/>
<property name="checkEmptyJavadoc" value="true"/>
<property name="checkEmptyJavadoc" value="false"/>
</module>
<module name="ConstantName">
<property name="severity" value="error"/>
Expand Down
9 changes: 8 additions & 1 deletion components/api/src/main/java/com/hotels/styx/api/ContentOverflowException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,14 @@
package com.hotels.styx.api;

/**
* An exception that indicates that a content-aggregation operation exceeded the maximum permitted amount of content.
* Indicates a content aggregation failure because an aggregator function received
* more data than expected.
* <p>
* A streaming HTTP message can be aggregated to a full message.
* The aggregator methods {@link HttpRequest#toFullRequest}, and
* {@link HttpResponse#toFullResponse} consume data from a network socket or
* some other unpredictable source and emit this exception when more data is
* received than expected.
*/
public class ContentOverflowException extends RuntimeException {
public ContentOverflowException(String message) {
Expand Down
2 changes: 1 addition & 1 deletion components/api/src/main/java/com/hotels/styx/api/FullHttpMessage.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
/**
* All behaviour common to both full requests and full responses.
*/
public interface FullHttpMessage {
interface FullHttpMessage {
/**
* Returns the protocol version of this.
*
Expand Down
118 changes: 78 additions & 40 deletions components/api/src/main/java/com/hotels/styx/api/FullHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,30 @@
import static java.util.stream.Stream.concat;

/**
* HTTP request with a fully aggregated/decoded body.
* An immutable HTTP request object including full body content.
* <p>
* A {@link FullHttpRequest} is useful for requests with a
* finite body content, such as when PUT or POST are used to create or
* modify a RESTful object.
* <p>
* A FullHttpRequest is created via {@link FullHttpRequest.Builder}. A new builder
* can be obtained by a call to following static methods:
*
* <ul>
* <li>{@code get}</li>
* <li>{@code head}</li>
* <li>{@code post}</li>
* <li>{@code put}</li>
* <li>{@code delete}</li>
* <li>{@code patch}</li>
* </ul>
*
* A builder can also be created with one of the {@code Builder} constructors.
*
* FullHttpRequest is immutable. Once created it cannot be modified.
* However a request can be transformed to another using the {@link this#newBuilder}
* method. It creates a new {@link Builder} with all message properties and
* body content cloned in.
*/
public class FullHttpRequest implements FullHttpMessage {
private final Object id;
Expand Down Expand Up @@ -83,7 +106,7 @@ public class FullHttpRequest implements FullHttpMessage {
* Creates a request with the GET method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous text was intended to show that it was the same builder being returned, so that chained calls can be used to set the values. The new text only tells us the type, which we already know.

Is there some builder convention we should be following?

Copy link
Contributor Author

@mikkokar mikkokar Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The methods calls can still be chained even if the a new instance was returned.

Whether the builder class is mutable or immutable persistent data structure is an implementation aspect affecting all setter methods. I would describe this in the class level javadoc rather than separately in each method. Like I said it would make no difference to method chaining.

If you feel strongly about this then let's create another issue to specifically improve javadocs for the Builder classes. For now, the priority is to cover the most important gaps in the javadocs, namely 1) the HTTP message API, and (in subsequent PRs) 2) the interceptors API, and 3) application providers.

We haven't agreed on any conventions. Feel free to suggest one.

Is this the only remaining issue that is blocking this PR?

*/
public static Builder get(String uri) {
return new Builder(GET, uri);
Expand All @@ -93,7 +116,7 @@ public static Builder get(String uri) {
* Creates a request with the HEAD method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
*/
public static Builder head(String uri) {
return new Builder(HEAD, uri);
Expand All @@ -103,7 +126,7 @@ public static Builder head(String uri) {
* Creates a request with the POST method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
*/
public static Builder post(String uri) {
return new Builder(POST, uri);
Expand All @@ -113,7 +136,7 @@ public static Builder post(String uri) {
* Creates a request with the DELETE method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
*/
public static Builder delete(String uri) {
return new Builder(DELETE, uri);
Expand All @@ -123,7 +146,7 @@ public static Builder delete(String uri) {
* Creates a request with the PUT method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
*/
public static Builder put(String uri) {
return new Builder(PUT, uri);
Expand All @@ -133,22 +156,32 @@ public static Builder put(String uri) {
* Creates a request with the PATCH method.
*
* @param uri URI
* @return {@code this}
* @return {@link FullHttpRequest.Builder}
*/
public static Builder patch(String uri) {
return new Builder(PATCH, uri);
}

/**
* @return HTTP protocol version
*/
@Override
public HttpVersion version() {
return this.version;
}

/**
* @return all HTTP headers as an {@link HttpHeaders} instance
*/
@Override
public HttpHeaders headers() {
return headers;
}

/**
* @param name header name
* @return all values for a given HTTP header name or an empty list if the header is not present
*/
@Override
public List<String> headers(CharSequence name) {
return headers.getAll(name);
Expand All @@ -162,7 +195,7 @@ public List<String> headers(CharSequence name) {
* reference cannot be used to modify the message content.
* <p>
*
* @return Message body content
* @return message body content
*/
@Override
public byte[] body() {
Expand All @@ -176,8 +209,8 @@ public byte[] body() {
* The caller must ensure the provided charset is compatible with message content
* type and encoding.
*
* @param charset Charset used to decode message body.
* @return Message body as a String.
* @param charset Charset used to decode message body
* @return message body as a String
*/
@Override
public String bodyAs(Charset charset) {
Expand All @@ -187,36 +220,28 @@ public String bodyAs(Charset charset) {
}

/**
* Gets the unique ID for this request.
*
* @return request ID
* @return an unique request ID
*/
public Object id() {
return id;
}

/**
* Returns the HTTP method of this request.
*
* @return the HTTP method
*/
public HttpMethod method() {
return method;
}

/**
* Returns the requested URI (or alternatively, path).
*
* @return The URI being requested
* @return the request URL
*/
public Url url() {
return url;
}

/**
* Returns the requested path.
*
* @return the path being requested
* @return the request URL path component
*/
public String path() {
return url.path();
Expand All @@ -235,11 +260,9 @@ public boolean keepAlive() {
}

/**
* Checks if the request has been transferred over a secure connection. If the protocol is HTTPS and the
* content is delivered over SSL then the request is considered to be secure.
*
* @return true if the request is transferred securely
* @deprecated will be removed from the final 1.0 API release
*/
@Deprecated
public boolean isSecure() {
return secure;
}
Expand Down Expand Up @@ -281,7 +304,7 @@ public Map<String, List<String>> queryParams() {
/**
* Get the names of all query parameters.
*
* @return the names of all query parameters.
* @return the names of all query parameters
*/
public Iterable<String> queryParamNames() {
return url.queryParamNames();
Expand All @@ -301,10 +324,10 @@ public Builder newBuilder() {
/**
* Converts this request into a streaming form (HttpRequest).
* <p>
* Converts this request into a HttpRequest object which represents the HTTP request as a
* Converts this request into an HttpRequest object which represents the HTTP request as a
* stream of bytes.
*
* @return A streaming HttpRequest object.
* @return A streaming HttpRequest object
*/
public HttpRequest toStreamingRequest() {
HttpRequest.Builder streamingBuilder = new HttpRequest.Builder(this)
Expand All @@ -321,7 +344,7 @@ public HttpRequest toStreamingRequest() {
/**
* Decodes the "Cookie" header in this request and returns the cookies.
*
* @return cookies
* @return a set of cookies
*/
public Set<RequestCookie> cookies() {
return headers.get(COOKIE)
Expand All @@ -333,7 +356,7 @@ public Set<RequestCookie> cookies() {
* Decodes the "Cookie" header in this request and returns the specified cookie.
*
* @param name cookie name
* @return cookies
* @return an optional cookie
*/
public Optional<RequestCookie> cookie(String name) {
return cookies().stream()
Expand Down Expand Up @@ -369,19 +392,34 @@ public static final class Builder {
private HttpVersion version = HTTP_1_1;
private byte[] body;

/**
* Creates a new {@link Builder} object with default attributes.
*/
public Builder() {
this.url = Url.Builder.url("/").build();
this.headers = new HttpHeaders.Builder();
this.body = new byte[0];
}

/**
* Creates a new {@link Builder} object with specified and URI.
*
* @param method a HTTP method
* @param uri URI
*/
public Builder(HttpMethod method, String uri) {
this();
this.method = requireNonNull(method);
this.url = Url.Builder.url(uri).build();
this.secure = url.isSecure();
}

/**
* Creates a new {@link Builder} from streaming request and a content byte array.
*
* @param request a streaming HTTP request object
* @param body an HTTP body content array
*/
public Builder(HttpRequest request, byte[] body) {
this.id = request.id();
this.method = request.method();
Expand Down Expand Up @@ -421,7 +459,7 @@ public Builder uri(String uri) {
* charset, and sets the Content-Length header accordingly.
*
* @param content request body
* @param charset Charset for string encoding.
* @param charset Charset for string encoding
* @return {@code this}
*/
public Builder body(String content, Charset charset) {
Expand All @@ -436,7 +474,7 @@ public Builder body(String content, Charset charset) {
* argument is true.
*
* @param content request body
* @param charset Charset used for encoding request body.
* @param charset charset used for encoding request body
* @param setContentLength If true, Content-Length header is set, otherwise it is not set.
* @return {@code this}
*/
Expand Down Expand Up @@ -508,8 +546,8 @@ public Builder headers(HttpHeaders headers) {
* <p/>
* Will not replace any existing values for the header.
*
* @param name The name of the header
* @param value The value of the header
* @param name the name of the header
* @param value the value of the header
* @return {@code this}
*/
public Builder addHeader(CharSequence name, Object value) {
Expand Down Expand Up @@ -566,7 +604,7 @@ public Builder method(HttpMethod method) {
* Sets the cookies on this request by overwriting the value of the "Cookie" header.
*
* @param cookies cookies
* @return this builder
* @return {@code this}
*/
public Builder cookies(RequestCookie... cookies) {
return cookies(asList(cookies));
Expand All @@ -576,7 +614,7 @@ public Builder cookies(RequestCookie... cookies) {
* Sets the cookies on this request by overwriting the value of the "Cookie" header.
*
* @param cookies cookies
* @return this builder
* @return {@code this}
*/
public Builder cookies(Collection<RequestCookie> cookies) {
requireNonNull(cookies);
Expand All @@ -595,7 +633,7 @@ public Builder cookies(Collection<RequestCookie> cookies) {
* add all new cookies in one call to the method rather than spreading them out.
*
* @param cookies new cookies
* @return this builder
* @return {@code this}
*/
public Builder addCookies(RequestCookie... cookies) {
return addCookies(asList(cookies));
Expand All @@ -608,7 +646,7 @@ public Builder addCookies(RequestCookie... cookies) {
* add all new cookies in one call to the method rather than spreading them out.
*
* @param cookies new cookies
* @return this builder
* @return {@code this}
*/
public Builder addCookies(Collection<RequestCookie> cookies) {
requireNonNull(cookies);
Expand All @@ -622,7 +660,7 @@ public Builder addCookies(Collection<RequestCookie> cookies) {
* Removes all cookies matching one of the supplied names by overwriting the value of the "Cookie" header.
*
* @param names cookie names
* @return this builder
* @return {@code this}
*/
public Builder removeCookies(String... names) {
return removeCookies(asList(names));
Expand All @@ -632,7 +670,7 @@ public Builder removeCookies(String... names) {
* Removes all cookies matching one of the supplied names by overwriting the value of the "Cookie" header.
*
* @param names cookie names
* @return this builder
* @return {@code this}
*/
public Builder removeCookies(Collection<String> names) {
requireNonNull(names);
Expand Down
Loading