Several NPEs under certain edge case conditions

[kazetsukaimiko]

Please run the tests under my edge case repository:
https://github.com/kazetsukaimiko/edgecases

More comments to follow.

[kazetsukaimiko]

Deletions in particular lead to problems in two known cases. First is:
com.example.nitrite.support.HappyFamilyTest.testDeleteIteratorNPE.

The ObjectFilter passed to the llamas ObjectRepository matches no documents. As a result, the nitriteIdList field in WriteResultImpl is null when you go to get the iterator of NitriteIds affected, causing an NPE.

[anidotnet]

Thanks for reporting it. I'll take a look into your test cases and provide fixes.

[kazetsukaimiko]

On a side note, Java 7 is not going to be supported forever (upon further reading, Oracle ceased updating Java 7 branches at the end of 2015). At your next major version you should at least consider a jump to Java 11, the next LTS release. Optional, when employed universally, forces you to deal with scenarios where your result may be null/empty and it will make your APIs easier to consume.

[kazetsukaimiko]

The second issue is that the DogCrudTest fails entirely. The failure is:
org.dizitart.no2.exceptions.NotIdentifiableException: NO2.8003: update operation failed as no id value found for the object

This is because Dog.class does not have @InheritIndices on it. You may argue this is required, that's not extremely obvious in your documentation and looking at the stack and how the Id field is detected, I question how necessary it is to require the annotation when I have the Nitrite @id annotation on the superclass.

[kazetsukaimiko]

Note that DogCrud and CatCrud (with minor tweaks to PetCrud) can actually share a common ObjectRepository! This is a particularly cool feature of how Nitrite uses Jackson by default (See the Pet class- the commented out @JsonTypeInfo annotation).

There are other problems with this test case I'd like to highlight later on if that is something you'd like to support. It does seem... edgy.

[kazetsukaimiko]

The other NPE is com.example.nitrite.support.HappyFamilyTest.testDelete.

I save an entity, delete it with my CRUD service, then try to find it with ObjectRepository::getById. This very clearly illustrates a case where Optional would simplify your internals, and not just bolster your API (getById itself should arguably return Optional). Because the underlying Document is null, the internals run into an NPE condition when searching for an object that no longer exists.

That does it for now, let me know if you have any questions.

[kazetsukaimiko]

Another NPE in my application from removing an entity, seems like the same thing on deletes. remove(T entity)- upon calling iterator, the NPE is thrown.

I can't actually get NitriteIds from the WriteResult objects that are results of deletions, it seems.

[anidotnet]

Thanks again for your test cases. These testDeleteIteratorNPE and testDelete were a good find :)

Now about the case of Optional. I know the benefit of it and I really wish it would have been there at much earlier version of Java. But unfortunately a large number of nitrite audiences use it for Android where Optional had been introduced from Api level 24 (i.e. Android 7) but the minimum api level required for nitrite is 19, so my hands are tied there.

@InheritIndices annotation I used to limit the class scanning using reflection. As nitrite is also used in resource constraint systems like android, I wanted to minimise the reflection overhead, the same reason nitrite has Mappable interface to bypass jackson's heavy reflection use.

You can use Pet class to create a single repository if you un-comment @JsonType.

@JsonTypeInfo(use=JsonTypeInfo.Id.CLASS, include=JsonTypeInfo.As.PROPERTY, property="@entity")
public abstract class Pet {
 ...
}

@Test
public void test() {
    ObjectRepository<Pet> pets = db.getRepository(Pet.class);
    Pet dog = new Dog();
    dog.setName("doggy");

    Pet cat = new Cat();
    cat.setName("meow");

    pets.insert(cat, dog);

    Cursor<Pet> cursor = pets.find(ObjectFilters.eq("name", "doggy"));
    assertEquals(cursor.size(), 1);
    Pet pet = cursor.firstOrDefault();
    assertTrue(pet instanceof Dog);
}

The fix for those NPEs are currently in 3.4.0-SNAPSHOTS.

[anidotnet]

To test the latest snapshot build, update your pom.xml as below

<dependency>
      <groupId>org.dizitart</groupId>
      <artifactId>nitrite</artifactId>
      <version>3.4.0-SNAPSHOT</version>
    </dependency>

....

<repositories>
    <repository>
      <id>oss.sonatype.org-snapshot</id>
      <url>http://oss.sonatype.org/content/repositories/snapshots</url>
      <releases>
        <enabled>false</enabled>
      </releases>
      <snapshots>
        <enabled>true</enabled>
      </snapshots>
    </repository>
  </repositories>

[kazetsukaimiko]

Understood on Android support, I kinda realized that after posting. On the web services side of Java (EE, Spring, etc) its almost a smell not to sell out heavily on use of Stream / Optional. Also, as I understand it reflection is more costly on Android. We (the "Java team") are finding ourselves in a similar/worse position than Python did with the 2.7 -> 3.x leap in some ways.

Thanks for the quick turnaround. I'll change to use 3.4.0-SNAPSHOT in my application until that sees a release.

A side note, the one commit mentioned in this bug doesn't contain the actual fixes in WriteResultImpl (I can see them in IntelliJ) that solve the NPE on iterator. I would have liked to have seen the whole diff if possible. One (constructive!) criticism I'd like to offer you is that your tests lean towards the happy-path. If you prefer, the next time I file any issues/bugs I could submit a PR with unit tests in nitrite instead of a standalone project. I thought about doing that this time, I didn't want to make you pull in a branch.

[anidotnet]

A side note, the one commit mentioned in this bug doesn't contain the actual fixes in WriteResultImpl (I can see them in IntelliJ) that solve the NPE on iterator. I would have liked to have seen the whole diff if possible.

You are right. The change regarding WriteResultImpl NPE went in another commit where I made some changes in gradle. Here is the diff - 78579ff

I admit, I have not dealt with enough negative test cases due to time constraint. But I'll be happy to accept any PR regarding test cases.

I am currently working on a refactored version of nitrite (4.x) in a private repo, later I'll merge it here when it is complete. If you want, I can give you access to that repo, so you can contribute there directly.

[kazetsukaimiko]

Thanks, I see the line.
4.x access: I'd prefer read-only if anything, as I can fork and submit PRs. Direct write access, I'd rather not.

Negative unit tests: Would you prefer test cases against 4.x, or against 3.x? How about some performance tests?

[anidotnet]

I'd prefer all test cases against 4.x as I am planning to freeze development on 3.x soon. I have made the repo public, so that you can submit a PR.

Repo link - https://github.com/anidotnet/nitrite-java/. Remember several apis have changed and no documentation yet. Existing test cases are only help for you as of now.

Performance tests would be fantastic. @sheinbergon has created some benchmark in kotlin here - https://github.com/sheinbergon/kotlintlv-nitrite-demo/. May be you can start from there. Currently I don't wish to include perf test code in main nitrite repo. If possible create a separate repo for perf test targeting 3.x now, later we can also compare 3.x vs 4.x and migrate that repo to nitrite's own org. How does the plan sound?

[kazetsukaimiko]

Great, I'll look at that tonight and start a branch to try and get some n-path tests going. Understood on documentation.

For performance testing, I'd propose that we include such tests in the repo. My first idea on how to tackle this:

-All @test methods generate a performance benchmark using @beforeeach @AfterEach JUnit hooks. If we pass a certain JVM Argument (-DmetricsFile) to the test JVM, we write the results for all tests to a file. If we pass the argument with another flag (-DcompareMetrics) @AfterEach compares the test to the values in that file, if it exists.
-To run performance tests, first a developer would checkout master (or a tag representing the latest release if master isn't guaranteed clean) and run all tests with the first argument, generating performance benchmarks. The developer would then checkout their own branch and run tests again, adding the second argument.
-If the performance metric for any test increases in execution time by a certain set percentage (like jacoco coverage), the test fails in the @AfterEach. This can be tuned as needed and will allow the developer to find cases where their branch introduces performance regressions.

The merits to this approach:
-All current and future tests contribute to performance testing.
-Performance tests are largely divorced from differences in hardware. You are comparing one branch to another on the same device so there's no need for complex scaling of results.
-You most importantly gain regression testing for performance that is easily repeatable and based on objective criteria.
-Your performance testing is official and in-repo without need for external projects or dependencies.
-This approach omits performance testing altogether by default, so it doesn't change your current workflow. Performance testing is still optional, enforced by discipline.

[kazetsukaimiko]

metricsFile could most simply point to a Nitrite Database file, containing entities that represent performance data (map entires of test class + test method to execution time in millis).

I can spin up a repo with a demonstration of this approach if you would like.

[anidotnet]

The approach sounds interesting. But my concern is that each Junit test will run only for one iteration, whereas to get proper perf metrics there should be some warm-up phase and each test should run for several iterations to capture proper data. One iteration of junit test will definitely not reflect actual perf metrics.

Its better to write separate JMH test suites to capture perf metrics. I would like to have it on a separate repo, so that people can extend it by adding other tests without affecting nitrite code.

[sheinbergon]

Hi @kazetsukaimiko

I'll just jump in and say that JUnit is ill-fitted for performance measurements, due to the optimizing nature of the JVM, warmup times, DCE, forking, outliers, etc.

Also, benchmarks are, well, benchmarks, not tests, and should not be treated as such (maybe not the right place to open that debate)

JMH is the best solution out-there to achieve what you aim to do.
Here's the repo path for the JMH benchmarks I crafted - https://github.com/sheinbergon/kotlintlv-nitrite-demo/tree/master/benchmarks
I can offer my help with getting started with it, should you need it.

Good luck

[kazetsukaimiko]

Sounds good guys.