chore: fixes multiple bugs (#338)

Author: mariuskatcontentful

src/main/java/com/contentful/java/cda/CDAClient.java

@@ -495,15 +495,19 @@ public Map<String, CDAContentType> apply(Response<CDAArray> arrayResponse) {
   }
 
   Flowable<CDAContentType> cacheTypeWithId(String id) {
-    CDAContentType contentType = cache.types().get(id);
+    Map<String, CDAContentType> types = cache.types();
+    CDAContentType contentType = types == null ? null : types.get(id);
     if (contentType == null) {
       return observe(CDAContentType.class)
               .one(id)
               .map(new Function<CDAContentType, CDAContentType>() {
                      @Override
                      public CDAContentType apply(CDAContentType resource) {
                        if (resource != null) {
-                         cache.types().put(resource.id(), resource);
+                         Map<String, CDAContentType> currentTypes = cache.types();
+                         if (currentTypes != null) {
+                           currentTypes.put(resource.id(), resource);
+                         }
                        }
                        return resource;
                      }
@@ -580,7 +584,7 @@ public static class Builder {
 
     boolean preview;
 
-    private boolean logSensitiveData = true;
+    private boolean logSensitiveData = false;
     Tls12Implementation tls12Implementation = useRecommendation;
 
     Section application;

src/main/java/com/contentful/java/cda/CDAHttpException.java

@@ -42,6 +42,9 @@ public CDAHttpException(Request request, Response response, boolean logSensitive
   }
 
   private String readResponseBody(Response response) {
+    if (response.body() == null) {
+      return "<no response body>";
+    }
     try {
       BufferedSource bufferedSource = response.body().source();
       Timeout timeout = bufferedSource.timeout();

src/main/java/com/contentful/java/cda/Cache.java

@@ -19,7 +19,9 @@ List<CDALocale> locales() {
   }
 
   protected CDALocale defaultLocale() {
-    return defaultLocale;
+    synchronized (localesLock) {
+      return defaultLocale;
+    }
   }
 
   void setLocales(List<CDALocale> locales) {
@@ -35,6 +37,7 @@ void updateDefaultLocale() {
       for (final CDALocale locale : this.locales) {
         if (locale.isDefaultLocale()) {
           this.defaultLocale = locale;
+          break;
         }
       }
     }

src/main/java/com/contentful/java/cda/ObserveQuery.java

@@ -5,6 +5,8 @@
 import org.reactivestreams.Publisher;
 import retrofit2.Response;
 
+import java.util.Map;
+
 import static com.contentful.java.cda.CDAType.LOCALE;
 import static com.contentful.java.cda.CDAType.TAG;
 import static com.contentful.java.cda.CDAType.ASSET;
@@ -67,7 +69,10 @@ public Flowable<T> one(final String id) {
     if (CONTENTTYPE.equals(typeForClass(type))) {
       flowable = flowable.map(t -> {
         if (t != null) {
-          client.cache.types().put(t.id(), (CDAContentType) t);
+          Map<String, CDAContentType> types = client.cache.types();
+          if (types != null) {
+            types.put(t.id(), (CDAContentType) t);
+          }
         }
         return t;
       });

src/main/java/com/contentful/java/cda/rich/RichTextFactory.java

@@ -178,6 +178,9 @@ private static void resolveRichLink(ArrayResource array, CDAEntry entry, CDAFiel
 
         for (final String locale : rawValue.keySet()) {
             final CDARichDocument document = entry.getField(locale, field.id());
+            if (document == null) {
+                continue;
+            }
             for (final CDARichNode node : document.getContent()) {
                 resolveOneLink(array, field, locale, node);
             }
@@ -276,8 +279,8 @@ private static boolean isLink(Object data) {
             final String id = (String) sys.get("id");
 
             if ("Link".equals(type)
-                    && ("Entry".equals(linkType) || "Asset".equals(linkType)
-                    && id != null)) {
+                    && ("Entry".equals(linkType) || "Asset".equals(linkType))
+                    && id != null) {
                 return true;
             }
         } catch (ClassCastException cast) {

src/test/java/com/contentful/java/cda/ClientTest.java

@@ -10,13 +10,19 @@
 import org.junit.Test;
 
 import java.io.IOException;
+import java.util.Map;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicReference;
 
 import okhttp3.Call;
 import okhttp3.Headers;
 import okhttp3.Interceptor;
 import okhttp3.OkHttpClient;
 import okhttp3.Response;
+import okhttp3.mockwebserver.MockResponse;
 import okhttp3.mockwebserver.RecordedRequest;
 
 import static com.contentful.java.cda.SyncType.onlyEntriesOfType;
@@ -353,6 +359,40 @@ public void requestingWhileRateLimitedThrows() {
     }
   }
 
+  // Regression test: response.body() can be null for a body-less unsuccessful response (e.g. an
+  // upstream proxy returning a bare 500/502 with no content). readResponseBody() in
+  // CDAHttpException used to dereference response.body() without a null check, throwing an
+  // unrelated NullPointerException that masked the real HTTP error. HTTP 204/304 are treated as
+  // successful by Retrofit/OkHttp and never reach CDAHttpException, so this is reproduced via a
+  // body-less 5xx error response instead.
+  @Test
+  @Enqueue
+  public void bodyLess500ResponseDoesNotThrowNpeAndProducesMeaningfulException() {
+    server.enqueue(new MockResponse().setResponseCode(500));
+
+    try {
+      client.fetch(CDAEntry.class).all();
+      throw new AssertionError("Expected CDAHttpException to be thrown");
+    } catch (CDAHttpException cdaException) {
+      assertThat(cdaException.responseCode()).isEqualTo(500);
+      assertThat(cdaException.responseBody()).isNotNull();
+    }
+  }
+
+  @Test
+  @Enqueue
+  public void bodyLess502ResponseDoesNotThrowNpeAndProducesMeaningfulException() {
+    server.enqueue(new MockResponse().setResponseCode(502));
+
+    try {
+      client.fetch(CDAEntry.class).all();
+      throw new AssertionError("Expected CDAHttpException to be thrown");
+    } catch (CDAHttpException cdaException) {
+      assertThat(cdaException.responseCode()).isEqualTo(502);
+      assertThat(cdaException.responseBody()).isNotNull();
+    }
+  }
+
   @Test(expected = IllegalArgumentException.class)
   @Enqueue("demo/content_types_cat.json")
   public void settingNoLoggerAndAnyLogLevelResultsException() {
@@ -381,6 +421,87 @@ public void clearingTheCacheClearsTheCache() {
     assertThat(client.cache.locales()).isNull();
   }
 
+  // Regression test: cache.types() returns null right after clearCache() is called.
+  // CDAClient#cacheTypeWithId(String) and the content-type caching step in
+  // ObserveQuery#one(String) used to dereference that null directly, causing an unhandled
+  // NullPointerException.
+  @Test
+  @Enqueue(defaults = {}, value = {
+      "demo/locales.json",
+      "demo/content_types_cat.json",
+      "demo/content_types_cat.json",
+      "demo/locales.json",
+      "demo/content_types_cat.json",
+      "demo/content_types_cat.json"
+  })
+  public void cacheTypeWithIdAfterClearCacheDoesNotThrow() {
+    // populate the cache first
+    client.fetch(CDAContentType.class).all();
+    assertThat(client.cache.types()).isNotNull();
+
+    client.clearCache();
+    // Preserve the existing, tested contract: cache.types() is null right after clear().
+    assertThat(client.cache.types()).isNull();
+
+    CDAContentType result = client.cacheTypeWithId("cat").blockingFirst();
+
+    assertThat(result).isNotNull();
+    assertThat(result.id()).isEqualTo("cat");
+  }
+
+  // Regression test: concurrent Cache#clear() and Cache#types() reads must never throw, since
+  // clearCache() is a public API that can legitimately race with any in-flight observable chain.
+  @Test
+  public void concurrentClearAndReadOfCacheTypesDoesNotThrow() throws InterruptedException {
+    final Cache cache = new Cache();
+    final int iterations = 500;
+    final CountDownLatch start = new CountDownLatch(1);
+    final AtomicReference<Throwable> failure = new AtomicReference<>();
+    ExecutorService executor = Executors.newFixedThreadPool(2);
+
+    Runnable clearer = () -> {
+      try {
+        start.await();
+        for (int i = 0; i < iterations; i++) {
+          cache.clear();
+        }
+      } catch (Throwable t) {
+        failure.compareAndSet(null, t);
+      }
+    };
+
+    Runnable reader = () -> {
+      try {
+        start.await();
+        for (int i = 0; i < iterations; i++) {
+          Map<String, CDAContentType> types = cache.types();
+          if (types != null) {
+            types.get("any-id");
+          }
+        }
+      } catch (Throwable t) {
+        failure.compareAndSet(null, t);
+      }
+    };
+
+    executor.submit(clearer);
+    executor.submit(reader);
+    start.countDown();
+
+    executor.shutdown();
+    executor.awaitTermination(5, TimeUnit.SECONDS);
+
+    assertThat(failure.get()).isNull();
+  }
+
+  // Regression test: the builder used to default logSensitiveData to `true`, leaking
+  // authorization headers into CDAHttpException#toString() output by default.
+  @Test
+  public void logSensitiveDataDefaultsToFalse() {
+    CDAClient defaultClient = createBuilder().build();
+    assertThat(defaultClient.shouldLogSensitiveData()).isFalse();
+  }
+
   @Test
   @Enqueue("demo/content_types_cat.json")
   public void localesGetCached() {

src/test/java/com/contentful/java/cda/RichTextTest.java

@@ -24,6 +24,31 @@
 import static com.google.common.truth.Truth.assertThat;
 
 public class RichTextTest extends BaseTest {
+
+  // Regression test: RichTextFactory.resolveRichLink() used to call document.getContent() on
+  // the result of entry.getField(locale, field.id()) without checking for null. That method
+  // returns null whenever a rich text field is present in the content model but not populated
+  // (and has no locale fallback configured) for a given locale - a common situation in partially
+  // localised spaces - causing an unhandled NullPointerException that failed the entire fetch,
+  // even for locales where the field was populated correctly.
+  @Test
+  @Enqueue(
+      value = "rich_text/entry_missing_locale.json",
+      defaults = {"rich_text/locales_two.json", "rich_text/content_types.json"}
+  )
+  public void richTextFieldMissingForOneLocaleDoesNotThrowAndOtherLocaleResolvesFine() {
+    final CDAEntry entry = (CDAEntry) client.fetch(CDAEntry.class).all().items().get(0);
+
+    final CDARichDocument populatedLocale = entry.getField("en-US", "rich");
+    assertThat(populatedLocale).isNotNull();
+    assertThat(populatedLocale.getContent()).isNotEmpty();
+
+    // de-DE has no fallback locale configured and the raw field value was explicitly null -
+    // it must resolve to a safely-skipped/unresolved value, not throw.
+    final CDARichDocument unpopulatedLocale = entry.getField("de-DE", "rich");
+    assertThat(unpopulatedLocale).isNull();
+  }
+
   @Test
   @Enqueue(value = "rich_text/simple_headline_1.json", defaults = {"rich_text/locales.json", "rich_text/content_types.json"})
   public void simple_headline_1_test() {

src/test/java/com/contentful/java/cda/rich/RichTextFactoryIsLinkTest.java

@@ -0,0 +1,65 @@
+package com.contentful.java.cda.rich;
+
+import org.junit.Test;
+
+import java.lang.reflect.Method;
+import java.util.HashMap;
+import java.util.Map;
+
+import static com.google.common.truth.Truth.assertThat;
+
+/**
+ * Regression tests for {@link RichTextFactory}'s private {@code isLink(Object)} method.
+ * <p>
+ * A prior operator-precedence defect (missing parentheses around a {@code ||} sub-expression)
+ * meant {@code id != null} only guarded the "Asset" branch, so an "Entry" link with a null id
+ * incorrectly returned {@code true}, later causing {@code link.data} to be silently set to
+ * {@code null} and crashing any renderer that dereferenced it as a {@code CDAEntry}.
+ * <p>
+ * Uses reflection to reach the private method directly, since it is not otherwise exposed.
+ */
+public class RichTextFactoryIsLinkTest {
+
+  private boolean invokeIsLink(Map<String, Object> data) throws Exception {
+    Method method = RichTextFactory.class.getDeclaredMethod("isLink", Object.class);
+    method.setAccessible(true);
+    return (boolean) method.invoke(null, data);
+  }
+
+  private Map<String, Object> link(String linkType, String id) {
+    Map<String, Object> sys = new HashMap<>();
+    sys.put("type", "Link");
+    sys.put("linkType", linkType);
+    if (id != null) {
+      sys.put("id", id);
+    }
+    Map<String, Object> data = new HashMap<>();
+    data.put("sys", sys);
+    return data;
+  }
+
+  @Test
+  public void entryLinkWithNullIdIsRejected() throws Exception {
+    assertThat(invokeIsLink(link("Entry", null))).isFalse();
+  }
+
+  @Test
+  public void assetLinkWithNullIdIsRejected() throws Exception {
+    assertThat(invokeIsLink(link("Asset", null))).isFalse();
+  }
+
+  @Test
+  public void entryLinkWithValidIdIsAccepted() throws Exception {
+    assertThat(invokeIsLink(link("Entry", "someEntryId"))).isTrue();
+  }
+
+  @Test
+  public void assetLinkWithValidIdIsAccepted() throws Exception {
+    assertThat(invokeIsLink(link("Asset", "someAssetId"))).isTrue();
+  }
+
+  @Test
+  public void unknownLinkTypeIsRejected() throws Exception {
+    assertThat(invokeIsLink(link("Space", "someId"))).isFalse();
+  }
+}