Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/main/java/com/contentful/java/cda/CDAClient.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -580,7 +584,7 @@ public static class Builder {

boolean preview;

private boolean logSensitiveData = true;
private boolean logSensitiveData = false;
Tls12Implementation tls12Implementation = useRecommendation;

Section application;
Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/contentful/java/cda/CDAHttpException.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
5 changes: 4 additions & 1 deletion src/main/java/com/contentful/java/cda/Cache.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ List<CDALocale> locales() {
}

protected CDALocale defaultLocale() {
return defaultLocale;
synchronized (localesLock) {
return defaultLocale;
}
}

void setLocales(List<CDALocale> locales) {
Expand All @@ -35,6 +37,7 @@ void updateDefaultLocale() {
for (final CDALocale locale : this.locales) {
if (locale.isDefaultLocale()) {
this.defaultLocale = locale;
break;
}
}
}
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/com/contentful/java/cda/ObserveQuery.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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) {
Expand Down
121 changes: 121 additions & 0 deletions src/test/java/com/contentful/java/cda/ClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down
25 changes: 25 additions & 0 deletions src/test/java/com/contentful/java/cda/RichTextTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
Loading