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.

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

Add Moshi #2182

Merged
merged 2 commits into from
Sep 25, 2023
Merged

Add Moshi #2182

merged 2 commits into from
Sep 25, 2023

Conversation

bhurghundii
Copy link
Contributor

@bhurghundii bhurghundii commented Sep 25, 2023

  • Add Moshi as a module to Feign
  • Create a MoshiEncoder and MoshiDecoder
  • Create tests for encoding and decoding, including an example using Github

Hello,

This PR adds Moshi as a module in Feign. It comes with an implemented encode and decode method which uses Moshi as well as a constructor to add your own custom adapter. I wrote tests using two different ways one can create a JSONAdapter using Moshi.

This is in response to an issue I opened on #2176.

Summary by CodeRabbit

  • New Feature: Added support for encoding and decoding JSON using the Moshi library in Feign clients. This includes new MoshiEncoder and MoshiDecoder classes that can be configured with a Feign.Builder instance.
  • New Feature: Provided custom JSON adapter functionality through constructors in MoshiEncoder and MoshiDecoder that accept a collection of JsonAdapter instances.
  • Test: Added comprehensive test coverage for the new Moshi encoder and decoder classes, including tests for custom encoders.
  • Documentation: Updated README with instructions on how to use the new Moshi encoder and decoder in a Feign client.

@coderabbitai
Copy link

coderabbitai bot commented Sep 25, 2023

Walkthrough

This pull request introduces Moshi support for JSON encoding and decoding in Feign. It adds MoshiEncoder and MoshiDecoder classes, along with their respective tests. The changes also include a MoshiFactory class for creating Moshi instances, and example usage in the GithubExample class.

Changes

File Summary
moshi/src/main/java/feign/moshi/MoshiDecoder.java
moshi/src/main/java/feign/moshi/MoshiEncoder.java
moshi/src/main/java/feign/moshi/MoshiFactory.java
Introduces MoshiEncoder and MoshiDecoder classes for JSON encoding and decoding using Moshi. Also adds a MoshiFactory for creating Moshi instances.
moshi/src/test/java/feign/moshi/MoshiDecoderTest.java
moshi/src/test/java/feign/moshi/MoshiEncoderTest.java
Adds test cases for MoshiEncoder and MoshiDecoder.
moshi/src/test/java/feign/moshi/... Adds various helper classes and data models used in the tests.
moshi/src/test/java/feign/moshi/examples/GithubExample.java Demonstrates the usage of the new Moshi encoder and decoder in a real-world scenario.
README.md
moshi/README.md
Updates documentation to explain the new Moshi support.

🐇💻

With a hop and a skip, we code and commit,

Adding Moshi to Feign, a perfect fit.

Encoding, decoding, all in a breeze,

Making HTTP requests as easy as cheese! 🧀🎉


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • Mention @coderabbitai in any review comment for bot assistance.
  • Note: Review comments are made on code diffs or files, not on the PR overview.

Pause Incremental Reviews

  • Insert @coderabbitai: ignore in the PR description to halt the bot's ongoing reviews. Remove the line to resume.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 9

Commits Files that changed from the base of the PR and between ec5aae5 and a849687.
Files ignored due to filter (2)
  • moshi/pom.xml
  • pom.xml
Files selected for processing (11)
  • README.md (1 hunks)
  • moshi/README.md (1 hunks)
  • moshi/src/main/java/feign/moshi/MoshiDecoder.java (1 hunks)
  • moshi/src/main/java/feign/moshi/MoshiEncoder.java (1 hunks)
  • moshi/src/main/java/feign/moshi/MoshiFactory.java (1 hunks)
  • moshi/src/test/java/feign/moshi/MoshiDecoderTest.java (1 hunks)
  • moshi/src/test/java/feign/moshi/MoshiEncoderTest.java (1 hunks)
  • moshi/src/test/java/feign/moshi/UpperZoneJSONAdapter.java (1 hunks)
  • moshi/src/test/java/feign/moshi/VideoGame.java (1 hunks)
  • moshi/src/test/java/feign/moshi/Zone.java (1 hunks)
  • moshi/src/test/java/feign/moshi/examples/GithubExample.java (1 hunks)
Files skipped from review due to trivial changes (5)
  • README.md
  • moshi/src/test/java/feign/moshi/MoshiDecoderTest.java
  • moshi/src/test/java/feign/moshi/MoshiEncoderTest.java
  • moshi/src/test/java/feign/moshi/VideoGame.java
  • moshi/src/test/java/feign/moshi/Zone.java
Additional comments (Suppressed): 2
moshi/src/main/java/feign/moshi/MoshiFactory.java (1)
  • 30-30: The add method of Moshi.Builder takes two parameters: a type and a JSON adapter for that type. Here, you're passing the class of the adapter itself as the type, which might not be what you intend. You should pass the class of the type that the adapter handles instead. Please verify if this is the intended behavior.
moshi/src/main/java/feign/moshi/MoshiDecoder.java (1)
  • 30-73: The MoshiDecoder class is well implemented. It provides multiple constructors to allow for different levels of customization of the Moshi instance used for decoding. The decode method handles various response scenarios appropriately, including 404 and 204 status codes, and null bodies. Error handling is also done correctly, with a specific catch block for JsonDataException that rethrows any underlying IOException. The reader is ensured to be closed in a finally block, which is good practice to prevent resource leaks.

moshi/README.md Outdated
Comment on lines 1 to 20
Moshi Codec
===================

This module adds support for encoding and decoding JSON via the Moshi library.

Add `MoshiEncoder` and/or `MoshiDecoder` to your `Feign.Builder` like so:

```java
GitHub github = Feign.builder()
.encoder(new MoshiEncoder())
.decoder(new MoshiDecoder())
.target(GitHub.class, "https://api.github.com");
```

### Map<String, Object> and Numbers
The default constructors of `MoshiEncoder` and `MoshiDecoder` decoder numbers in
`Map<String, Object>` as Integer type. This prevents reading `{"counter", "1"}`
as `Map.of("counter", 1.0)`.

To change this, please use constructors that accept a Moshi object.
Copy link

Choose a reason for hiding this comment

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

The documentation update is clear and provides a good overview of how to use the new Moshi encoder and decoder. It also explains the default behavior for number decoding in Map<String, Object> and how to change it. However, it would be beneficial to include an example of using custom JsonAdapter instances with the MoshiEncoder and MoshiDecoder, as mentioned in the PR summary.

Comment on lines +24 to +35

public static void main(String... args) {
GitHub github = Feign.builder().encoder(new MoshiEncoder())
.decoder(new MoshiDecoder())
.target(GitHub.class, "https://api.github.com");

System.out.println("Let's fetch and print a list of the contributors to this library.");
List<Contributor> contributors = github.contributors("netflix", "feign");
for (Contributor contributor : contributors) {
System.out.println(contributor.login + " (" + contributor.contributions + ")");
}
}
Copy link

Choose a reason for hiding this comment

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

The main method in the GithubExample class is directly making a network call to fetch contributors from GitHub. This could potentially block the main thread if the network call takes too long. Consider moving this network call to a separate thread or using asynchronous programming techniques.

Comment on lines +43 to +47
static class Contributor {

String login;
int contributions;
}
Copy link

Choose a reason for hiding this comment

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

The Contributor class is currently using package-private fields for login and contributions. While this might work within the context of this example, it's generally a good practice to encapsulate these fields with getter and setter methods to maintain data integrity and control over access.

Comment on lines +24 to +44

private final Moshi moshi;

public MoshiEncoder() {
this.moshi = new Moshi.Builder().build();
}

public MoshiEncoder(Moshi moshi) {
this.moshi = moshi;
}

public MoshiEncoder(Iterable<JsonAdapter<?>> adapters) {
this(MoshiFactory.create(adapters));
}

@Override
public void encode(Object object, Type bodyType, RequestTemplate template) {
JsonAdapter<Object> jsonAdapter = moshi.adapter(bodyType).indent(" ");
template.body(jsonAdapter.toJson(object));
}
}
Copy link

Choose a reason for hiding this comment

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

The MoshiEncoder class is well implemented. It provides three constructors to create an instance of the encoder with a default Moshi instance, a custom Moshi instance, or a set of JsonAdapter instances. The encode method correctly uses the Moshi instance to create a JsonAdapter for the body type and encode the object into JSON format. However, it's worth noting that the indent(" ") call in line 41 will add two spaces of indentation to the output JSON. This can increase the size of the payload, especially for large objects. If this is not a requirement, consider removing it to optimize the performance.

-    JsonAdapter<Object> jsonAdapter = moshi.adapter(bodyType).indent("  ");
+    JsonAdapter<Object> jsonAdapter = moshi.adapter(bodyType);

Comment on lines +26 to +34
static Moshi create(Iterable<JsonAdapter<?>> adapters) {
Moshi.Builder builder = new Moshi.Builder();

for (JsonAdapter<?> adapter : adapters) {
builder.add(adapter.getClass(), adapter);
}

return builder.build();
}
Copy link

Choose a reason for hiding this comment

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

The create method is declared as static, which means it can be called without creating an instance of MoshiFactory. However, the method is package-private (default access modifier), which restricts its visibility to classes within the same package. If this method is intended to be used outside of the feign.moshi package, consider changing its access modifier to public.

- static Moshi create(Iterable<JsonAdapter<?>> adapters) {
+ public static Moshi create(Iterable<JsonAdapter<?>> adapters) {

Comment on lines +46 to +67
@Override
public Object decode(Response response, Type type) throws IOException {
JsonAdapter<Object> jsonAdapter = moshi.adapter(type);

if (response.status() == 404 || response.status() == 204)
return Util.emptyValueOf(type);
if (response.body() == null)
return null;

Reader reader = response.body().asReader(UTF_8);

try {
return parseJson(jsonAdapter, reader);
} catch (JsonDataException e) {
if (e.getCause() != null && e.getCause() instanceof IOException) {
throw (IOException) e.getCause();
}
throw e;
} finally {
ensureClosed(reader);
}
}
Copy link

Choose a reason for hiding this comment

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

Consider adding error handling for other potential HTTP status codes that may require special handling. For example, a 400 (Bad Request) or 500 (Internal Server Error) might need to be handled differently than a 404 or 204. This would make your decoder more robust and adaptable to different server responses.

    if (response.status() == 404 || response.status() == 204)
+    else if (response.status() >= 400)
+      throw new IOException("Server returned HTTP status code " + response.status());
      return Util.emptyValueOf(type);

Comment on lines +69 to +72
private Object parseJson(JsonAdapter<Object> jsonAdapter, Reader reader) throws IOException {
String targetString = CharStreams.toString(reader);
return jsonAdapter.fromJson(targetString);
}
Copy link

Choose a reason for hiding this comment

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

While converting the entire JSON body into a string before parsing it with Moshi works, it can be inefficient for large JSON bodies as it requires storing the entire JSON body in memory as a string. Instead, you can parse the JSON directly from the Reader which is more memory-efficient and can handle larger JSON bodies.

-    String targetString = CharStreams.toString(reader);
-    return jsonAdapter.fromJson(targetString);
+    return jsonAdapter.fromJson(reader);

Comment on lines +24 to +34
public void toJson(JsonWriter out, LinkedList<Zone> value) throws IOException {
out.beginArray();
for (Zone zone : value) {
out.beginObject();
for (Map.Entry<String, Object> entry : zone.entrySet()) {
out.name(entry.getKey()).value(entry.getValue().toString().toUpperCase());
}
out.endObject();
}
out.endArray();
}
Copy link

Choose a reason for hiding this comment

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

The toJson method does not handle null values. If the value parameter or any of the Zone objects within it are null, this will result in a NullPointerException. Consider adding null checks to prevent this.

  public void toJson(JsonWriter out, LinkedList<Zone> value) throws IOException {
+   if (value == null) {
+     out.nullValue();
+     return;
+   }
    out.beginArray();
    for (Zone zone : value) {
+     if (zone == null) {
+       out.nullValue();
+       continue;
+     }
      out.beginObject();
      for (Map.Entry<String, Object> entry : zone.entrySet()) {
        out.name(entry.getKey()).value(entry.getValue().toString().toUpperCase());
      }
      out.endObject();
    }
    out.endArray();
  }

Comment on lines +37 to +50
public LinkedList<Zone> fromJson(JsonReader in) throws IOException {
LinkedList<Zone> zones = new LinkedList<>();
in.beginArray();
while (in.hasNext()) {
in.beginObject();
Zone zone = new Zone();
while (in.hasNext()) {
zone.put(in.nextName(), in.nextString().toUpperCase());
}
in.endObject();
zones.add(zone);
}
in.endArray();
return zones;
Copy link

Choose a reason for hiding this comment

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

Similar to the toJson method, the fromJson method does not handle null values. If the JSON input contains null values, this could lead to unexpected behavior. Consider adding null checks to prevent this.

  public LinkedList<Zone> fromJson(JsonReader in) throws IOException {
    LinkedList<Zone> zones = new LinkedList<>();
    in.beginArray();
    while (in.hasNext()) {
      in.beginObject();
      Zone zone = new Zone();
      while (in.hasNext()) {
+       String name = in.nextName();
+       String value = in.nextString();
+       if (name != null && value != null) {
+         zone.put(name, value.toUpperCase());
+       }
      }
      in.endObject();
      zones.add(zone);
    }
    in.endArray();
    return zones;
  }

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Commits Files that changed from the base of the PR and between a849687 and 3fc3003.
Files selected for processing (1)
  • moshi/README.md (1 hunks)
Files skipped from review due to trivial changes (1)
  • moshi/README.md

@velo velo merged commit b5e6809 into OpenFeign:master Sep 25, 2023
1 check passed
velo pushed a commit that referenced this pull request Oct 7, 2024
* Add Moshi

* Update README.md

---------

Co-authored-by: Vikramaditya Chhapwale <[email protected]>
velo pushed a commit that referenced this pull request Oct 8, 2024
* Add Moshi

* Update README.md

---------

Co-authored-by: Vikramaditya Chhapwale <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants