Unexpected behaviour when getting Current Product?

[LEQADA]

I have to get the Current product even if it is unpublished. As a platform user I expect that I will just fetch the product from Products endpoint and then

product.getMasterData().getCurrent()

where product is an object created from io.sphere.sdk.products.Product.

However SDK decides instead of me that I shouldn't get the current product if it is unpublished

https://github.com/commercetools/commercetools-jvm-sdk/blob/8055d7a99ccd9126a6431151ccee061971850282/commercetools-models/src/main/java/io/sphere/sdk/products/ProductCatalogDataImpl.java#L23-L25

I think SDK should return exactly what I fetched from the platform and not add some custom logic layer on that.

[katmatt]

We implemented it this way to prevent that our users accidentially access unpublished invalid current data. Why do you have to access unpublished current data? What's your usecase?

[LEQADA]

@katmatt our customer wants to be able to show the product on some pages if it was unpublished let's say 10 days ago. I understand that it might be a breaking change to refactor this method. Maybe we can introduce a new method like getCurrentForced() to be able to manipulate with the current product even when it's unpublished? Because that is what anyone will expect from the SDK and not the current behaviour.

[katmatt]

I like the idea to introduce a new method and we internally discussed it already. How about calling it getCurrentUnsafe?

[LEQADA]

@katmatt I would avoid using Unsafe here, because there is actually nothing unsafe in a product fetching. Forced is better from my perspective because we can't get an unpublished product but we can force it to get. For instance Apache is using force argument to access a private field of the object with reflection https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/reflect/FieldUtils.html#readField-java.lang.reflect.Field-java.lang.Object-boolean-

[ulvimardaliyev]

I would like to work on this issue. Is it still needed? Do you have any tips for me before I start?

[LEQADA]

@commercetools/java-admins we have a volunteer. What is the state of this issue?

[LEQADA]

ping @katmatt @acbeni

[acbeni]

@ulvimardaliyev @LEQADA sorry for the late replay, actually i have only one remark for the naming, i think that getCurrentUnpublished is more straight forward and clear WDYT @LEQADA @ulvimardaliyev @katmatt ?

[LEQADA]

@acbeni current can't be unpublished. current is just a projection of a product. Product can have a state of published or unpublished. So I think we should not stick product's state to a projection.

[acbeni]

Ok @LEQADA , since none of the methods is suggestive, i would go with getCurrentUnsafe and rely more on the javadoc more to signal why its called so.

[ulvimardaliyev]

@LEQADA, @acbeni when I cloned the project to my laptop and opened it on Idea IDE, saw that there was not a class ProductCatalogDataImplBase. IDE does not suggest to import it. How can I get that class? Could you help me, please?

[ulvimardaliyev]

And, when I wrote mvn clean install on command prompt there was an error. I created a gist for an error, From here you can get it.

[acbeni]

@ulvimardaliyev the ProductCatalogDataImplBase will be generated during the build process, in order to compile first with no tests try mvn clean install -DskipTests

[ulvimardaliyev]

@acbeni I've also checked mvn clean install -DskipTests, unfortunately, error occurs again.

[acbeni]

the taglet library is tightly coupled with java 8, maybe try setting JAVA_HOME to a java 8 library , and retry

[ulvimardaliyev]

On my machine, there is the latest version of Java 8 and JAVA_HOME is addressed to C:\Program Files\Java\jdk1.8.0_191 so, as I understood, should I change that directory to Java/jdk/lib folder?

[acbeni]

No i think your java home setup is fine, add this flag -Dmaven.javadoc.skip=true to skip javadc, and see if it works.

[LEQADA]

@acbeni can it be a bug of some mvn plugin which can't find java because of the whitespace in the path? Just guessing

[acbeni]

not sure to be hones but just for java case we can use ./mvnw clean install -DskipTests -Dmaven.javadoc.skip=true , because it might be the maven version that is causing problem.

[ulvimardaliyev]

@acbeni, @LEQADA, I have checked ./mvnw clean install -DskipTests -Dmaven.javadoc.skip=true, but another error occurs on commercetools-tests-fragment ....................... FAILURE

[LEQADA]

Just tried on Windows 7 x64 and got the same error

>mvnw -version
Apache Maven 3.5.0 (ff8f5e7444045639af65f6095c62210b5713f426; 2017-04-03T22:39:0
6+03:00)
Maven home: C:\Users\Test\.m2\wrapper\dists\apache-maven-3.5.0-bin\6ps54u5pnnbbp
r6ds9rppcc7iv\apache-maven-3.5.0
Java version: 1.8.0_191, vendor: Oracle Corporation
Java home: C:\Program Files\Java\jdk1.8.0_191\jre
OS name: "windows 7", version: "6.1", arch: "amd64", family: "windows"

[LEQADA]

@commercetools/java-admins I think it's worth a separate issue. We're obviously lacking of system requirements for the project development in the documentation.

[ulvimardaliyev]

@acbeni, advised by @LEQADA to check those commands on Linux, I have installed Ubuntu Terminal from Windows Store and checked both mvn clean install -DskipTests -Dmaven.javadoc.skip=true and mvn clean install -DskipTests. Happily, on terminal there was a BUILD SUCCESS message. Then, I opened the project, but, unfortunately, there is no a class ProductCatalogDataImplBase yet.

[acbeni]

@ulvimardaliyev check on the following path {projectRoot}/commercetools-models/target/generated-sources/annotations/io/sphere/sdk/products/ProductCatalogDataImplBase.java you should find it there

[ulvimardaliyev]

@acbeni and @LEQADA, since our last conversation, I had not been able to import the ProductCatalogDataImplBase class. After searching a lot, finally, I found this great help from stackoverflow, so changing <scope> from provided to compile on pom.xml is the best solution. Now, it is time to work on this project :smiley:

[LEQADA]

@ulvimardaliyev did you fix it on Windows?

[ulvimardaliyev]

@LEQADA no, I have changed my OS from Windows to Linux, Ubuntu. I will try to fix this maven problem on Windows, them, will comment about the result.

[ulvimardaliyev]

@LEQADA, I checked on Windows 10, from the terminal of Intellij Idea Ultimate version 2018.2.5, unfortunately, Build Failureoccurred. But, using Ubuntu Terminal on Windows which is downloaded from Windows Store, fixes all problems and on the same IDE, there were no errors, no importing notifications about ProductCatalogDataImplBaseclass.

[ulvimardaliyev]

@LEQADA, @acbeni, I have cloned the project again, then registered on https://admin.commercetools.com/ and got the credentials. After that, I added integrationtest.properties file to the project. Then I ran mvn clean install (also ./mvnw clean install) and some integration tests failed in commercetools-models. You can check those failures here. Any tips on how to fix it?

[LEQADA]

ping @acbeni @katmatt

[cenkakin]

this issue also causes a problem for us. In our business, we are using the current version of a product to display in the shop as expected and we keep showing it even it is unpublished after some time. To check the data consistency between the shop and ctp data, we would like to reach the current version when we query the product and I like the idea to introduce a new method for getting it.

We would be grateful if you can give this issue more priority @katmatt @acbeni .

[katmatt]

@ulvimardaliyev We really appreciate that you want to contribute to our project and we plan to work on #1839 in the near future. The failures in our tests that you linked can happen from time to time and just show that the timeout in our integration tests aren't high enough. Our plan is to fix #1839 in the near future and thus make contributing to our JVM SDK easier in the future. Overall this issue is more about naming and the actual fix is trivial: You just have to add a method that just returns the current field. And thus it makes more sense for us to work on it on our own instead out sourcing it. @ulvimardaliyev Sorry that it took so long for us to make up our mind, but we hope that you're fine if we provide a solution with one of our next releases.

[katmatt]

@cenkakin We discussed this in our planning today and will add it to the milestone for our next release.

[LEQADA]

Overall this issue is more about naming and the actual fix is trivial

this is exactly why this issue is a good match for new contributors

And thus it makes more sense for us to work on it on our own instead out sourcing it

Because it is so easy? Maintaining an open source library means involving contributors, supporting them.

[katmatt]

@LEQADA No, because it needs some agreement on our side and if we have this agreement it's easier for us to just fix it. But I agree with you that we should make it easier to contribute to our project and that's why we will work on #1839.