Skip to content

Location Parent NPE#38

Merged
sbesson merged 11 commits intoome:masterfrom
sbesson:location_parent_npe
Feb 6, 2019
Merged

Location Parent NPE#38
sbesson merged 11 commits intoome:masterfrom
sbesson:location_parent_npe

Conversation

@sbesson
Copy link
Copy Markdown
Member

@sbesson sbesson commented Jan 14, 2019

See #12, #14 and https://trello.com/c/Ym1HWlQG/73-ome-common-location-exception

This PR resurrect the Location.getParentFile() NPE fix proposed in #12 but pushed away as potentially breaking now we are converging towards 6.0.0.

  • reintroduces the unit tests from Location NPE fix #12 showing the new expected behavior
  • updates the Javadoc to reuse java.io.File as Location is a direct extension/replacement of this class
  • re-introduces the fix for the new behavior (getParentFile() return null if getParent is null rather than throwing a NPE). Incidentally, Add S3Handle #33 fixed one of the 2 constructors and c4e54a0 fixes the second one.

@joshmoore
Copy link
Copy Markdown
Member

This no longer needs the rest of the changes from ome/bioformats#2914?

@melissalinkert
Copy link
Copy Markdown
Member

Seconding the concern in #38 (comment), but otherwise changes here all look fine and make sense to me.

@sbesson
Copy link
Copy Markdown
Member Author

sbesson commented Jan 25, 2019

As discussed recently, my expectation is that without ome/bioformats#2914, this change would not put us in a worse place. I would expect that:

  • datasets for which a reader would throw a NPE while invoking Location.getParentFile would return null with this API and likely fail with another NPE at a later stage
  • all other cases should remain unaffected by this change

Obviously, proving these two assertions is the tricky part:

  • automated tests have been passing with this PR included. This is semi-expected as per construction none of the datasets are mounted directly under /
  • options for testing the first case would involved either setting up am image with a dataset under / or testing e.g. under some Windows system with some datasets under a mount point

I might try and get back to setting up a testing environment. If that is not the case, it might be equally fine to release ome-common:6.0.0 without this PR and schedule it as part of a future major release. I do not think we have a concrete use case which depends on it, this was mostly API cleanup.

@melissalinkert
Copy link
Copy Markdown
Member

Discussed briefly with @sbesson earlier today. For a concrete test case, I copied perkinelmer-operetta/hms/ryang_skbr3_p1__2012-07-10T15_24_30-Measurement1/Images/* to D:\ on my local Windows machine, so that Index.idx.xml did not have a grandparent directory. git grep getParentFile().getParentFile() in https://github.com/openmicroscopy/bioformats/tree/develop/components/formats-gpl/src/loci/formats/in was helpful in finding a reader to target.

With this included:

D:\> showinf -nopix Index.idx.xml
Exception in thread "main" java.lang.NullPointerException
              at loci.formats.in.OperettaReader.initFile(OperettaReader.java:264)
              at loci.formats.FormatReader.setId(FormatReader.java:1395)
              at loci.formats.ImageReader.setId(ImageReader.java:842)
              at loci.formats.ReaderWrapper.setId(ReaderWrapper.java:650)
              at loci.formats.tools.ImageInfo.testRead(ImageInfo.java:1035)
              at loci.formats.tools.ImageInfo.main(ImageInfo.java:1121)

Without this change:

D:\> showinf -nopix Index.idx.xml
Exception in thread "main" java.lang.NullPointerException
              at java.io.File.<init>(File.java:277)
              at loci.common.Location.<init>(Location.java:237)
              at loci.common.Location.<init>(Location.java:169)
              at loci.common.Location.getParentFile(Location.java:873)
              at loci.formats.in.OperettaReader.initFile(OperettaReader.java:263)
              at loci.formats.FormatReader.setId(FormatReader.java:1395)
              at loci.formats.ImageReader.setId(ImageReader.java:842)
              at loci.formats.ReaderWrapper.setId(ReaderWrapper.java:650)
              at loci.formats.tools.ImageInfo.testRead(ImageInfo.java:1035)
              at loci.formats.tools.ImageInfo.main(ImageInfo.java:1121)

That supports the arguments that:

  1. it takes targeted effort or a very non-standard workflow to get an NPE related to this PR
  2. any NPE triggered by this PR simply replaces a different NPE

I'd be fine with having this merged for 6.0.0, and including the checks in ome/bioformats#2914 (plus any others as needed, since that diff is now over a year old) in a later patch release.

@sbesson sbesson added this to the 6.0.0 milestone Feb 4, 2019
@sbesson sbesson merged commit f6124be into ome:master Feb 6, 2019
@sbesson sbesson deleted the location_parent_npe branch February 6, 2019 14:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants