438: Reference resolution of Product references (and set of) as an attribute of nested type #439
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…roduct references and set of them.
Codecov Report
@@ Coverage Diff @@
## master #439 +/- ##
===========================================
+ Coverage 98.67% 98.7% +0.02%
+ Complexity 1498 1493 -5
===========================================
Files 131 131
Lines 3787 3771 -16
Branches 203 202 -1
===========================================
- Hits 3737 3722 -15
Misses 32 32
+ Partials 18 17 -1
Continue to review full report at Codecov.
|
ahmetoz
suggested changes
Sep 5, 2019
src/main/java/com/commercetools/sync/products/helpers/VariantReferenceResolver.java
Outdated
Show resolved
Hide resolved
...tReferenceResolver/attributes/nested-attribute-with-set-of-product-reference-attributes.json
Outdated
Show resolved
Hide resolved
src/test/java/com/commercetools/sync/products/helpers/VariantReferenceResolverTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/commercetools/sync/products/helpers/VariantReferenceResolverTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/commercetools/sync/products/helpers/VariantReferenceResolverTest.java
Outdated
Show resolved
Hide resolved
...mercetools/sync/integration/externalsource/products/ProductSyncWithReferencedProductsIT.java
Show resolved
Hide resolved
...mercetools/sync/integration/externalsource/products/ProductSyncWithReferencedProductsIT.java
Outdated
Show resolved
Hide resolved
...mercetools/sync/integration/externalsource/products/ProductSyncWithReferencedProductsIT.java
Show resolved
Hide resolved
JudeNiroshan
reviewed
Sep 5, 2019
| @@ -184,19 +168,11 @@ static boolean isProductReference(@Nonnull final JsonNode referenceValue) { | |||
| } | |||
|
|
|||
| @Nonnull | |||
| CompletionStage<Optional<String>> getResolvedIdFromKeyInReference(@Nonnull final JsonNode referenceValue) { | |||
| CompletionStage<Optional<String>> getProductResolvedIdFromKeyInReference(@Nonnull final JsonNode referenceValue) { | |||
| final JsonNode idField = referenceValue.get(REFERENCE_ID_FIELD); | |||
| return idField != null | |||
There was a problem hiding this comment.
Shouldn't we handle the idField.isNull() case here?
There was a problem hiding this comment.
Really good question. Even though, implementation wise, It would still work without this check. Because eventually, if it doesn't find "null" in the cache, will try to fetch it, and won't find it, so will still return empty optional.
However, having the check as you suggested is better as it avoids having to fetch at all. - Kind of short-circuiting.
ahmetoz
approved these changes
Sep 5, 2019
JudeNiroshan
approved these changes
Sep 5, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses #438
Important to Note
The test
resolveAttributeReference_WithNullReferenceInSetAttribute_ShouldResolveReferencesis disabled due to a possible bug on FasterXML/jackson-databind#2442. If this PR is ready to be merged, we can create an issue for this and merge this. --> #441Todo