Jackrabbit Oak
  1. Jackrabbit Oak
  2. OAK-37

Use nullability annotation to enforce/document API contract

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.7
    • Component/s: core, jcr
    • Labels:
      None

      Description

      In a discussion about exception handling on the dev list [1] Julian brough up the idea of using nullability annotations in APIs. I think we should decide on which one to use and start using them whereever apropriate.

      1. nullability.patch
        8 kB
        Julian Reschke
      2. nullability.patch
        8 kB
        Julian Reschke

        Issue Links

          Activity

          Hide
          Julian Reschke added a comment -

          I just checked with FindBugs 2.01, and annotating methods with @CheckForNull and @Nonnull seemed to have the desired effect.

          That being said, this appears to be a can of worms; JSR-305 is dormant; FindBugs supports the annotation using it's own package + javax.annotation.

          IntelliJ is said to support them in "all" packages, but I can't test that.

          There's also a Eclipse JDT extension in the works which appears to be configurable with respect to package names (http://wiki.eclipse.org/JDT_Core/Null_Analysis)

          In the end we need something that has an acceptable license, and can be automatically checked; FindBugs is probably a candidate for that.

          Show
          Julian Reschke added a comment - I just checked with FindBugs 2.01, and annotating methods with @CheckForNull and @Nonnull seemed to have the desired effect. That being said, this appears to be a can of worms; JSR-305 is dormant; FindBugs supports the annotation using it's own package + javax.annotation. IntelliJ is said to support them in "all" packages, but I can't test that. There's also a Eclipse JDT extension in the works which appears to be configurable with respect to package names ( http://wiki.eclipse.org/JDT_Core/Null_Analysis ) In the end we need something that has an acceptable license, and can be automatically checked; FindBugs is probably a candidate for that.
          Hide
          Jukka Zitting added a comment -

          FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).

          IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.

          Show
          Jukka Zitting added a comment - FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do). IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.
          Hide
          Julian Reschke added a comment -

          FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do).

          For intra-subproject checking they are only needed at compile time. However, I'm not sure how it will work when we use a JAR from another subproject. But let's start simple.

          IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken.

          That should be possible but maybe I'll need some hand-holding with that.

          Show
          Julian Reschke added a comment - FindBugs annotations sound good to me as long as they don't introduce a mandatory runtime dependency on the code (which I don't think they do). For intra-subproject checking they are only needed at compile time. However, I'm not sure how it will work when we use a JAR from another subproject. But let's start simple. IDE support (IntelliJ, Eclipse, etc.) is nice, but IMHO not essential. The important bit is that we're able to configure our Maven build to check such annotations and fail the build if the explicitly declared rules are broken. That should be possible but maybe I'll need some hand-holding with that.
          Hide
          Julian Reschke added a comment -

          Note that the nullability annotations in edu.umd.cs.findbugs.annotations are marked as deprecated; so maybe we really should use the "JSR305" variant.

          Show
          Julian Reschke added a comment - Note that the nullability annotations in edu.umd.cs.findbugs.annotations are marked as deprecated; so maybe we really should use the "JSR305" variant.
          Hide
          Jukka Zitting added a comment -

          OK. I'm fine with whatever we can make work reasonably well. We can always adjust things later on if a better solution comes along.

          Show
          Jukka Zitting added a comment - OK. I'm fine with whatever we can make work reasonably well. We can always adjust things later on if a better solution comes along.
          Hide
          Julian Reschke added a comment -

          Minimal demonstration, showing how to embed the annotations.

          Note:

          • cross project checking with the FindBugs plugin in Eclipse indeed works
          • FindBugs report:
            Possible null pointer dereference in org.apache.jackrabbit.oak.jcr.security.user.UserManagerImpl.removeInternalProperty(Node, String) due to return value of called method UserManagerImpl.java /oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/user line 352 FindBugs Problem (Troubling)

          I believe this is something we should do; running FindBugs from the actual Maven build would be the next step

          Show
          Julian Reschke added a comment - Minimal demonstration, showing how to embed the annotations. Note: cross project checking with the FindBugs plugin in Eclipse indeed works FindBugs report: Possible null pointer dereference in org.apache.jackrabbit.oak.jcr.security.user.UserManagerImpl.removeInternalProperty(Node, String) due to return value of called method UserManagerImpl.java /oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/security/user line 352 FindBugs Problem (Troubling) I believe this is something we should do; running FindBugs from the actual Maven build would be the next step
          Hide
          Jukka Zitting added a comment -

          +1 great stuff!

          PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.

          Show
          Jukka Zitting added a comment - +1 great stuff! PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.
          Hide
          Julian Reschke added a comment -

          PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using.

          Done; see attachment. Should I go ahead and commit this, and see how it works in practice once we add more annotations?

          Show
          Julian Reschke added a comment - PS. The Maven dependencies should have a <scope>provided</scope> entry in them. Otherwise they'll show up as transitive dependencies in downstream components. See for example the bndlib dependency for an example of another annotation library we're using. Done; see attachment. Should I go ahead and commit this, and see how it works in practice once we add more annotations?
          Hide
          Jukka Zitting added a comment -

          Yes, just go ahead. It doesn't seem to break anything and produces useful information, so no reason to hold back.

          Show
          Jukka Zitting added a comment - Yes, just go ahead. It doesn't seem to break anything and produces useful information, so no reason to hold back.
          Hide
          Julian Reschke added a comment -

          For Eclipse, I recommend adding the FindBugs plugin. Update site: http://findbugs.cs.umd.edu/eclipse

          Show
          Julian Reschke added a comment - For Eclipse, I recommend adding the FindBugs plugin. Update site: http://findbugs.cs.umd.edu/eclipse
          Hide
          Jukka Zitting added a comment -

          I added the findbugs and checkstyle plugins to the pedantic profile of our Maven build in revision 1365017. For now they are configured to simply output their findings without causing the build to fail.

          Show
          Jukka Zitting added a comment - I added the findbugs and checkstyle plugins to the pedantic profile of our Maven build in revision 1365017. For now they are configured to simply output their findings without causing the build to fail.
          Hide
          Michael Dürig added a comment -

          Cleaning up old issues. Please reopen if necessary. Consider opening a new issue for follow up work.

          Show
          Michael Dürig added a comment - Cleaning up old issues. Please reopen if necessary. Consider opening a new issue for follow up work.

            People

            • Assignee:
              Julian Reschke
              Reporter:
              Michael Dürig
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development