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.