From 5eca0f5af98dfd2437bc5b82aeaabf55f59c7b9d Mon Sep 17 00:00:00 2001 From: Marius Kurgonas Date: Thu, 16 Jul 2026 15:54:34 +0300 Subject: [PATCH] chore: fixes multiple bugs --- .../com/contentful/java/cda/CDAClient.java | 10 +- .../contentful/java/cda/CDAHttpException.java | 3 + .../java/com/contentful/java/cda/Cache.java | 5 +- .../com/contentful/java/cda/ObserveQuery.java | 7 +- .../java/cda/rich/RichTextFactory.java | 7 +- .../com/contentful/java/cda/ClientTest.java | 121 ++++++++++++++++++ .../com/contentful/java/cda/RichTextTest.java | 25 ++++ .../cda/rich/RichTextFactoryIsLinkTest.java | 65 ++++++++++ .../rich_text/entry_missing_locale.json | 66 ++++++++++ src/test/resources/rich_text/locales_two.json | 32 +++++ 10 files changed, 334 insertions(+), 7 deletions(-) create mode 100644 src/test/java/com/contentful/java/cda/rich/RichTextFactoryIsLinkTest.java create mode 100644 src/test/resources/rich_text/entry_missing_locale.json create mode 100644 src/test/resources/rich_text/locales_two.json diff --git a/src/main/java/com/contentful/java/cda/CDAClient.java b/src/main/java/com/contentful/java/cda/CDAClient.java index b033d011..a4eb9325 100644 --- a/src/main/java/com/contentful/java/cda/CDAClient.java +++ b/src/main/java/com/contentful/java/cda/CDAClient.java @@ -495,7 +495,8 @@ public Map apply(Response arrayResponse) { } Flowable cacheTypeWithId(String id) { - CDAContentType contentType = cache.types().get(id); + Map types = cache.types(); + CDAContentType contentType = types == null ? null : types.get(id); if (contentType == null) { return observe(CDAContentType.class) .one(id) @@ -503,7 +504,10 @@ Flowable cacheTypeWithId(String id) { @Override public CDAContentType apply(CDAContentType resource) { if (resource != null) { - cache.types().put(resource.id(), resource); + Map 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; diff --git a/src/main/java/com/contentful/java/cda/CDAHttpException.java b/src/main/java/com/contentful/java/cda/CDAHttpException.java index d0958e5b..807b4f5c 100644 --- a/src/main/java/com/contentful/java/cda/CDAHttpException.java +++ b/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 ""; + } try { BufferedSource bufferedSource = response.body().source(); Timeout timeout = bufferedSource.timeout(); diff --git a/src/main/java/com/contentful/java/cda/Cache.java b/src/main/java/com/contentful/java/cda/Cache.java index f6e4e02d..a903b683 100644 --- a/src/main/java/com/contentful/java/cda/Cache.java +++ b/src/main/java/com/contentful/java/cda/Cache.java @@ -19,7 +19,9 @@ List locales() { } protected CDALocale defaultLocale() { - return defaultLocale; + synchronized (localesLock) { + return defaultLocale; + } } void setLocales(List locales) { @@ -35,6 +37,7 @@ void updateDefaultLocale() { for (final CDALocale locale : this.locales) { if (locale.isDefaultLocale()) { this.defaultLocale = locale; + break; } } } diff --git a/src/main/java/com/contentful/java/cda/ObserveQuery.java b/src/main/java/com/contentful/java/cda/ObserveQuery.java index 01fdcc33..8417d2b7 100644 --- a/src/main/java/com/contentful/java/cda/ObserveQuery.java +++ b/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 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 types = client.cache.types(); + if (types != null) { + types.put(t.id(), (CDAContentType) t); + } } return t; }); diff --git a/src/main/java/com/contentful/java/cda/rich/RichTextFactory.java b/src/main/java/com/contentful/java/cda/rich/RichTextFactory.java index a3a2afb0..4fba3598 100644 --- a/src/main/java/com/contentful/java/cda/rich/RichTextFactory.java +++ b/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) { diff --git a/src/test/java/com/contentful/java/cda/ClientTest.java b/src/test/java/com/contentful/java/cda/ClientTest.java index c12e1837..d84a343c 100644 --- a/src/test/java/com/contentful/java/cda/ClientTest.java +++ b/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 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 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() { diff --git a/src/test/java/com/contentful/java/cda/RichTextTest.java b/src/test/java/com/contentful/java/cda/RichTextTest.java index 76fed303..3f4d8774 100644 --- a/src/test/java/com/contentful/java/cda/RichTextTest.java +++ b/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() { diff --git a/src/test/java/com/contentful/java/cda/rich/RichTextFactoryIsLinkTest.java b/src/test/java/com/contentful/java/cda/rich/RichTextFactoryIsLinkTest.java new file mode 100644 index 00000000..7d28584f --- /dev/null +++ b/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. + *

+ * 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}. + *

+ * Uses reflection to reach the private method directly, since it is not otherwise exposed. + */ +public class RichTextFactoryIsLinkTest { + + private boolean invokeIsLink(Map data) throws Exception { + Method method = RichTextFactory.class.getDeclaredMethod("isLink", Object.class); + method.setAccessible(true); + return (boolean) method.invoke(null, data); + } + + private Map link(String linkType, String id) { + Map sys = new HashMap<>(); + sys.put("type", "Link"); + sys.put("linkType", linkType); + if (id != null) { + sys.put("id", id); + } + Map 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(); + } +} diff --git a/src/test/resources/rich_text/entry_missing_locale.json b/src/test/resources/rich_text/entry_missing_locale.json new file mode 100644 index 00000000..d0164b2f --- /dev/null +++ b/src/test/resources/rich_text/entry_missing_locale.json @@ -0,0 +1,66 @@ +{ + "sys": { + "type": "Array" + }, + "total": 1, + "skip": 0, + "limit": 100, + "items": [ + { + "sys": { + "space": { + "sys": { + "type": "Link", + "linkType": "Space", + "id": "" + } + }, + "id": "temp1BugFixVerification", + "type": "Entry", + "createdAt": "2018-10-17T13:32:30.252Z", + "updatedAt": "2019-01-04T14:06:02.180Z", + "environment": { + "sys": { + "id": "human-readable", + "type": "Link", + "linkType": "Environment" + } + }, + "revision": 3, + "contentType": { + "sys": { + "type": "Link", + "linkType": "ContentType", + "id": "rich" + } + } + }, + "fields": { + "name": { + "en-US": "entry_missing_locale" + }, + "rich": { + "en-US": { + "data": {}, + "content": [ + { + "data": {}, + "content": [ + { + "data": {}, + "marks": [], + "value": "Populated only for en-US", + "nodeType": "text" + } + ], + "nodeType": "paragraph" + } + ], + "nodeType": "document" + }, + "de-DE": null + } + } + } + ] +} diff --git a/src/test/resources/rich_text/locales_two.json b/src/test/resources/rich_text/locales_two.json new file mode 100644 index 00000000..a9b96a4d --- /dev/null +++ b/src/test/resources/rich_text/locales_two.json @@ -0,0 +1,32 @@ +{ + "sys": { + "type": "Array" + }, + "total": 2, + "skip": 0, + "limit": 1000, + "items": [ + { + "code": "en-US", + "name": "English (United States)", + "default": true, + "fallbackCode": null, + "sys": { + "id": "7fpOneSflazHm9rrNGxojE", + "type": "Locale", + "version": 1 + } + }, + { + "code": "de-DE", + "name": "German (Germany)", + "default": false, + "fallbackCode": null, + "sys": { + "id": "8fpOneSflazHm9rrNGxojF", + "type": "Locale", + "version": 1 + } + } + ] +}