Product Create API Responses with status 400 are not handled correctly

[Mercious]

When working with the 10.0.0-beta.4 (not tried with any other) version, any API responses with status 400 that indicate some problem with the data sent result in a NullPointerException. When logging the occurring error with an errorCallback Handler on the syncOptions, the actual issue with the request can not be seen anymore. The actual API response and the reason behind the 400 status code is lost.

It seems like that at least for product creation, the issue lies here: com.commercetools.sync.services.impl.BaseService#executeCreateCommand. The code here expects the createCommand to finish exceptionally if anything other than a 200 is returned. However, in reality a 400 response seems to not raise any exceptions. Instead, the returned result simply has a null-body. The original error-message is also not present anymore, so even debugging into here does not help. The following code will then produce a NPE, because it tries to put the results body into cache. But since that is null, the cache implementation throws a NPE because neither key or value is nullable. From roughly reading through the SDK documentation it would seem like their HandlerStack should translate any >=400 status codes into exceptions, but it does not seem to be the case. Whether or not this is an issue of the SDK itself or me missunderstanding it, I cannot say.

To Reproduce Here is a simple test-case of trying to sync an invalid product-draft because it's masterVariant uses an attribute that doesn't exist.

  final var unknownAttribute = AttributeBuilder.of()
          .name("this-doesnt-exist")
          .value("foo")
          .build();

  final var variantDraft = ProductVariantDraftBuilder.of()
          .key("test-product-variant")
          .sku("test-product-variant")
          .attributes(List.of(unknownAttribute))
          .build();

  final var invalidProductDraft = ProductDraftBuilder.of()
          .key("invalid-product-with-unknown-attribute")
          .name(LocalizedStringBuilder.of().addValue("en", "invalid-product-with-unknown-attribute").build())
          .slug(LocalizedStringBuilder.of().addValue("en", "invalid-product-with-unknown-attribute").build())
          .withProductType(builder -> builder.key("my-product-type-key").build())
          .masterVariant(variantDraft)
          .build();

  final var opts = ProductSyncOptionsBuilder.of(apiRoot)
          .errorCallback(((e, productDraft, productProjection, productUpdateActions) -> LOG.error("Product sync failed", e)))
          .build();

  final var result = new ProductSync(opts).sync(List.of(invalidProductDraft))
          .toCompletableFuture()
          .join();

This produces this log message:

Product sync failed

com.commercetools.sync.commons.exceptions.SyncException: Failed to process the ProductDraft with key:'invalid-product-with-unknown-attribute'. Reason: java.lang.NullPointerException at com.commercetools.sync.commons.BaseSync.handleError(BaseSync.java:164) ~[commercetools-sync-java-10.0.0-beta.4.jar:10.0.0-beta.4] at com.commercetools.sync.products.ProductSync.lambda$syncDraft$19(ProductSync.java:414) ~[commercetools-sync-java-10.0.0-beta.4.jar:10.0.0-beta.4] at java.base/java.util.concurrent.CompletableFuture.uniExceptionally(CompletableFuture.java:990) ~[na:na] at java.base/java.util.concurrent.CompletableFuture$UniExceptionally.tryFire(CompletableFuture.java:974) ~[na:na] at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510) ~[na:na] at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2147) ~[na:na] [. . .] Caused by: java.util.concurrent.CompletionException: java.lang.NullPointerException at java.base/java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[na:na] at java.base/java.util.concurrent.CompletableFuture.completeThrowable(CompletableFuture.java:320) ~[na:na] at java.base/java.util.concurrent.CompletableFuture.uniHandle(CompletableFuture.java:936) ~[na:na] at java.base/java.util.concurrent.CompletableFuture$UniHandle.tryFire(CompletableFuture.java:911) ~[na:na] ... 34 common frames omitted Caused by: java.lang.NullPointerException: null at java.base/java.util.Objects.requireNonNull(Objects.java:209) ~[na:na] at com.github.benmanes.caffeine.cache.BoundedLocalCache.put(BoundedLocalCache.java:2298) ~[caffeine-3.1.8.jar:3.1.8] at com.github.benmanes.caffeine.cache.BoundedLocalCache.put(BoundedLocalCache.java:2278) ~[caffeine-3.1.8.jar:3.1.8] at com.github.benmanes.caffeine.cache.LocalManualCache.put(LocalManualCache.java:126) ~[caffeine-3.1.8.jar:3.1.8] at com.commercetools.sync.services.impl.BaseService.lambda$executeCreateCommand$13(BaseService.java:237) ~[commercetools-sync-java-10.0.0-beta.4.jar:10.0.0-beta.4] at java.base/java.util.concurrent.CompletableFuture.uniHandle(CompletableFuture.java:934) ~[na:na] ... 35 common frames omitted

Expected behavior The SyncException that is passed to the errorCallback should contain useful information about what actually caused the ProductDraft to fail. Without this information it becomes very hard to understand what went wrong, especially if the issue is not reproducible by simply recreating the craft and sending it again.

[lojzatran]

Hi @Mercious

Thank you for reporting the issue.

We will check it and let you know the results.

[Mercious]

Hi @lojzatran ,

it turns out that this was an issue with how I built my ProjectApiRoot client. The SDK documentation is a bit vague and hard to understand regarding this (https://docs.commercetools.com/sdk/java-sdk-middleware#errormiddleware), but you have to build the ProjectApiRoot by calling withErrorMiddleware() first. Otherwise requests with >= 400 are not translated into exceptions and then the mentioned sync-library code does not properly handle 400 responses anymore. With the error-middleware in place, the logging seems to work properly, the thrown SyncException's contain the actual reasons of the API response.

[lojzatran]

Hi @Mercious

Good to hear that you solved the issue. I will close the issue then. Feel free to (re)open if you have more questions.

[jenschude]

@Mercious could you give more details about your issue with the error middleware? By default the ErrorMiddleware should be added when using one of the defaultClient methods. And it shouldn't be an issue in which order you call the method as the builder will ensure that it's added first in the middleware stack.

[Mercious]

@jenschude: Yeah, it looks like I built my ProjectApiRoot without the usage of the defaultClient builder method. I used a combination of withProjectKey / withApiBaseUrl / withClientCredentialsFlow instead and hence was missing the ErrorMiddleware. The SDK documentation (https://docs.commercetools.com/sdk/java-sdk-getting-started#create-the-client-class) does indeed use defaultClient, so one shouldn't run into this issue when following that closely. Although it never explicitly states the relevancy of using exactly that way of building the client in favour of one of the many other ways the builder class exposes. There is no mention of the connection between that builder and the middlewares it configures for you. But this indeed not an issue of the sync project, so all good - thanks for the answers!

[jenschude]

I may add a small section about the default middlewares which are initialized by the defaultClient