Makes iterator compatible with Java iterator expected behavior (OpenFeign#1117)

ItamarBenjamin · velo · commit ad8c9190ae1a · 2019-11-25T09:20:06.000+13:00
* Makes iterator compatible with Java iterator expected behavior
both next() and hasNext() should read from stream if needed. both also inspect 'current' member, next() resets it after consuming. exception is thrown when no more elements are available to return.

* Fixing CI - formatting issue
diff --git a/jackson/src/main/java/feign/jackson/JacksonIteratorDecoder.java b/jackson/src/main/java/feign/jackson/JacksonIteratorDecoder.java
@@ -15,13 +15,9 @@
 
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.JsonToken;
-import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.Module;
-import com.fasterxml.jackson.databind.ObjectMapper;
-import com.fasterxml.jackson.databind.ObjectReader;
-import com.fasterxml.jackson.databind.RuntimeJsonMappingException;
+import com.fasterxml.jackson.databind.*;
 import feign.Response;
-import feign.Util;
 import feign.codec.DecodeException;
 import feign.codec.Decoder;
 import java.io.BufferedReader;
@@ -32,6 +28,7 @@
 import java.lang.reflect.Type;
 import java.util.Collections;
 import java.util.Iterator;
+import java.util.NoSuchElementException;
 import static feign.Util.ensureClosed;
 
 /**
@@ -131,33 +128,47 @@ static final class JacksonIterator<T> implements Iterator<T>, Closeable {
 
     @Override
     public boolean hasNext() {
+      if (current == null) {
+        current = readNext();
+      }
+      return current != null;
+    }
+
+    private T readNext() {
       try {
         JsonToken jsonToken = parser.nextToken();
         if (jsonToken == null) {
-          return false;
+          return null;
         }
 
         if (jsonToken == JsonToken.START_ARRAY) {
           jsonToken = parser.nextToken();
         }
 
         if (jsonToken == JsonToken.END_ARRAY) {
-          current = null;
           ensureClosed(this);
-          return false;
+          return null;
         }
 
-        current = objectReader.readValue(parser);
+        return objectReader.readValue(parser);
       } catch (IOException e) {
         // Input Stream closed automatically by parser
         throw new DecodeException(response.status(), e.getMessage(), response.request(), e);
       }
-      return current != null;
     }
 
     @Override
     public T next() {
-      return current;
+      if (current != null) {
+        T tmp = current;
+        current = null;
+        return tmp;
+      }
+      T next = readNext();
+      if (next == null) {
+        throw new NoSuchElementException();
+      }
+      return next;
     }
 
     @Override
diff --git a/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java b/jackson/src/test/java/feign/jackson/JacksonIteratorTest.java
@@ -13,25 +13,27 @@
  */
 package feign.jackson;
 
-import static feign.Util.UTF_8;
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.hamcrest.core.Is.isA;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import feign.Request;
 import feign.Request.HttpMethod;
 import feign.Response;
 import feign.Util;
 import feign.codec.DecodeException;
 import feign.jackson.JacksonIteratorDecoder.JacksonIterator;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.ExpectedException;
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Collections;
 import java.util.LinkedHashMap;
+import java.util.NoSuchElementException;
 import java.util.concurrent.atomic.AtomicBoolean;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExpectedException;
+import static feign.Util.UTF_8;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.hamcrest.core.Is.isA;
 
 public class JacksonIteratorTest {
 
@@ -43,6 +45,31 @@ public void shouldDecodePrimitiveArrays() throws IOException {
     assertThat(iterator(Integer.class, "[0,1,2,3]")).containsExactly(0, 1, 2, 3);
   }
 
+  @Test
+  public void shouldNotSkipElementsOnHasNext() throws IOException {
+    JacksonIterator<Integer> iterator = iterator(Integer.class, "[0]");
+    assertThat(iterator.hasNext()).isTrue();
+    assertThat(iterator.hasNext()).isTrue();
+    assertThat(iterator.next()).isEqualTo(0);
+    assertThat(iterator.hasNext()).isFalse();
+  }
+
+  @Test
+  public void hasNextIsNotMandatory() throws IOException {
+    JacksonIterator<Integer> iterator = iterator(Integer.class, "[0]");
+    assertThat(iterator.next()).isEqualTo(0);
+    assertThat(iterator.hasNext()).isFalse();
+  }
+
+  @Test
+  public void expectExceptionOnNoElements() throws IOException {
+    JacksonIterator<Integer> iterator = iterator(Integer.class, "[0]");
+    assertThat(iterator.next()).isEqualTo(0);
+    assertThatThrownBy(() -> iterator.next())
+        .hasMessage(null)
+        .isInstanceOf(NoSuchElementException.class);
+  }
+
   @Test
   public void shouldDecodeObjects() throws IOException {
     assertThat(iterator(User.class, "[{\"login\":\"bob\"},{\"login\":\"joe\"}]"))
