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

        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