Add typed response

[gromspys]

Issue: #1627

Summary by CodeRabbit

• New Feature: Introduced TypedResponse class to handle different types of HTTP responses, enhancing the flexibility of the Feign library.
• Improvement: Updated the toString() method in the Response class for more accurate string representation of HTTP responses.
• Test: Added a new test method typedResponse() to validate the functionality of the new TypedResponse class.

These changes provide a more flexible and accurate way to handle and represent HTTP responses, improving the overall user experience with the Feign library.

[coderabbitai]

Walkthrough

The changes introduce a new class TypedResponse to handle typed responses in the Feign library, enhancing flexibility. The InvocationContext class has been updated to accommodate this new type. The Response class now uses dynamic protocol versioning. New tests have been added to validate these changes.

Changes

File	Summary
.../feign/InvocationContext.java	Updated proceed() method to handle TypedResponse. Added a new decode() method for response decoding.
.../feign/TypedResponse.java	Introduced a new class TypedResponse to handle typed responses. It includes a builder pattern and getter methods.
.../feign/Response.java	Modified toString() method to use dynamic protocol versioning.
.../feign/FeignTest.java	Added new test method typedResponse() to validate the changes. Added a new method getWithTypedResponse() to the TestInterface.

🐇
"In the land of code, changes are afoot,
A new class is born, and it's quite astute.
With typed responses, we now can play,
And dynamic versioning is now in the fray.
Tests are written, to keep us in check,
Celebrate these changes, they're quite tech!" 🎉

---

Tips

Chat with CodeRabbit Bot (@coderabbitai)

• If you reply to a review comment from CodeRabbit, the bot will automatically respond.
• To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
• Note: Review comments are made on code diffs or files, not on the PR overview.
• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• The JSON schema for the configuration file is available here.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.json

[coderabbitai]

Review Status

Actionable comments generated: 4

Commits

Files that changed from the base of the PR and between 25c419d and 80e0f0b.

Files selected for processing (3)

• core/src/main/java/feign/InvocationContext.java (2 hunks)
• core/src/main/java/feign/TypedResponse.java (1 hunks)
• core/src/test/java/feign/FeignTest.java (2 hunks)

Additional comments (Suppressed): 3

core/src/test/java/feign/FeignTest.java (1)

• 1216-1217: The new method getWithTypedResponse() in the TestInterface returns a TypedResponse<String>. Ensure that the implementation of this method in the actual class correctly wraps the response in a TypedResponse object. Also, verify that the generic type String is appropriate for the response body.

core/src/main/java/feign/InvocationContext.java (2)

• 80-91: The new code checks if the return type is assignable to TypedResponse. If true, it decodes the response and wraps it in a TypedResponse object. This is a significant change in the behavior of the proceed() method. Ensure that all parts of the codebase that use this method are compatible with this change. Also, verify that the TypedResponse class and its builder method are implemented correctly and tested thoroughly.

• 112-121: The decoding logic has been moved to a new private method decode(). This improves the modularity and readability of the code. However, ensure that this change does not affect the error handling logic in the proceed() method. Specifically, verify that exceptions thrown by the decode() method are caught and handled correctly in the proceed() method.

[coderabbitai]

The new test typedResponse() is well written and covers the new feature of handling typed responses. It checks the status and body of the response, which are the most critical parts of an HTTP response. However, it would be beneficial to add assertions for other fields of the TypedResponse object to ensure they are correctly populated. This will provide a more comprehensive test of the new feature.

assertEquals("HTTP/1.1", response.protocolVersion());
assertNotNull(response.headers());
assertNotNull(response.request());

[coderabbitai]

The request() method in the Builder class checks if the request argument is null. However, similar checks are not performed for other setter methods like status(), headers(), and body(). It's a good practice to validate method arguments to prevent invalid or unexpected values.

    public Builder status(int status) {
+      checkArgument(status >= 100 && status < 600, "status code must be valid");
      this.status = status;
      return this;
    }
...
    public Builder headers(Map<String, Collection<String>> headers) {
+      checkNotNull(headers, "headers must not be null");
      this.headers = headers;
      return this;
    }
...
    public Builder body(T body) {
+      checkNotNull(body, "body must not be null");
      this.body = body;
      return this;
    }

[coderabbitai]

The toString() method constructs a string representation of the TypedResponse object. However, it assumes that the protocol version is always HTTP/1.1, which might not be the case. Consider using the protocolVersion field to reflect the actual protocol version.

  public String toString() {
-    StringBuilder builder = new StringBuilder("HTTP/1.1 ").append(status);
+    StringBuilder builder = new StringBuilder(protocolVersion.toString()).append(' ').append(status);
    if (reason != null)
      builder.append(' ').append(reason);
    builder.append('\n');
    for (String field : headers.keySet()) {
      for (String value : valuesOrEmpty(headers, field)) {
        builder.append(field).append(": ").append(value).append('\n');
      }
    }
    if (body != null)
      builder.append('\n').append(body);
    return builder.toString();
  }

[coderabbitai]

Review Status

Actionable comments generated: 2

Configuration used: CodeRabbit UI

Commits

Files that changed from the base of the PR and between 80e0f0b and e71b886.

Files selected for processing (3)

• core/src/main/java/feign/Response.java (1 hunks)
• core/src/main/java/feign/TypedResponse.java (1 hunks)
• core/src/test/java/feign/FeignTest.java (3 hunks)

Files skipped from review due to trivial changes (1)

• core/src/main/java/feign/Response.java

Additional comments: 3

core/src/test/java/feign/FeignTest.java (3)

• 52-52: The import import static junit.framework.TestCase.assertNotNull; is added. Ensure that this import is used in the test cases and is not redundant.

• 92-105: A new test case typedResponse() is added. This test case is testing the new functionality of handling typed responses. It checks the status, body, protocol version of the response, and also verifies that headers and request are not null. The test case seems to be well written and covers the necessary assertions for the new functionality.

• 1220-1222: A new method getWithTypedResponse() is added to the TestInterface interface. This method returns a TypedResponse<String>, which is the new functionality being tested. Ensure that this method is implemented in the classes that implement this interface.

[coderabbitai]

The constructor of TypedResponse still lacks validation for essential fields like status and headers. This could lead to the creation of invalid TypedResponse objects. Please consider adding checks for these fields.

  private TypedResponse(Builder<T> builder) {
    checkState(builder.request != null, "original request is required");
+    checkState(builder.status >= 100 && builder.status < 600, "status code must be valid");
+    checkState(builder.headers != null, "headers must not be null");
    this.status = builder.status;
    this.request = builder.request;
    this.reason = builder.reason; // nullable
    this.headers = caseInsensitiveCopyOf(builder.headers);
    this.body = builder.body;
    this.protocolVersion = builder.protocolVersion;
  }

Committable suggestion (Beta)

Suggested change

	checkState(builder.request != null, "original request is required");
	this.status = builder.status;
	this.request = builder.request;
	this.reason = builder.reason; // nullable
	this.headers = caseInsensitiveCopyOf(builder.headers);
	this.body = builder.body;
	this.protocolVersion = builder.protocolVersion;
	}
	private TypedResponse(Builder<T> builder) {
	checkState(builder.request != null, "original request is required");
	checkState(builder.status >= 100 && builder.status < 600, "status code must be valid");
	checkState(builder.headers != null, "headers must not be null");
	this.status = builder.status;
	this.request = builder.request;
	this.reason = builder.reason; // nullable
	this.headers = caseInsensitiveCopyOf(builder.headers);
	this.body = builder.body;
	this.protocolVersion = builder.protocolVersion;
	}

[coderabbitai]

The setter methods in the Builder class (status(), headers(), body()) still lack validation for their arguments. This could lead to the creation of invalid TypedResponse objects. Please consider adding checks for these methods.

    public Builder status(int status) {
+      checkArgument(status >= 100 && status < 600, "status code must be valid");
      this.status = status;
      return this;
    }
...
    public Builder headers(Map<String, Collection<String>> headers) {
+      checkNotNull(headers, "headers must not be null");
      this.headers = headers;
      return this;
    }
...
    public Builder body(T body) {
+      checkNotNull(body, "body must not be null");
      this.body = body;
      return this;
    }

Committable suggestion (Beta)

Suggested change

	public Builder body(T body) {
	this.body = body;
	return this;
	}
	public Builder status(int status) {
	checkArgument(status >= 100 && status < 600, "status code must be valid");
	this.status = status;
	return this;
	}
	...
	public Builder headers(Map<String, Collection<String>> headers) {
	checkNotNull(headers, "headers must not be null");
	this.headers = headers;
	return this;
	}
	...
	public Builder body(T body) {
	checkNotNull(body, "body must not be null");
	this.body = body;
	return this;
	}