Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ND2 defensive parsing #691

Merged
merged 1 commit into from
Sep 11, 2013
Merged

Conversation

ctrueden
Copy link
Member

@ctrueden ctrueden commented Sep 5, 2013

This change is an attempted fix for Fiji bug #655.

Several numerical parsing methods throw NumberFormatException when the input string is not actually a number. In particular:

  • Integer.parseInt(String)
  • Double.parseDouble(String)
  • new Integer(String)
  • new Double(String)

The ND2 handler parses a large number of string key/value pairs, converting values into numbers in many cases. But in almost every case, no error handling was done in case the input value was not a number.

This change adds a blanket try/catch for NumberFormatException around areas that parse lots of numbers, emitting a warning for any non-conforming key/value pairs. This allows parsing to continue, hopefully resulting in a less fragile ND2 reader.

Note that this change is nearly all whitespace indentation; the easiest way to view the "real" change is using git show -b on the commit.

@melissalinkert
Copy link
Member

--test nd2

@melissalinkert
Copy link
Member

Excluding, as this caused http://hudson.openmicroscopy.org.uk/view/Bio-Formats/job/BIOFORMATS-full-repository-stable/335/ to fail. Most likely, wrapping all of the metadata parsing in a single try/catch block prevents something from executing, but what exactly is not obvious from the diff. For a more manageable example to test locally, this should result in 11 test failures with this PR's branch:

ant -Dtestng.memory=4096m -Dtestng.directory=/ome/data_repo/from_skyking/nd2/jonas test-automated

The same test with dev_4_4 alone results in 0 failures.

Several numerical parsing methods throw NumberFormatException when
the input string is not actually a number. In particular:

* Integer.parseInt(String)
* Double.parseDouble(String)
* new Integer(String)
* new Double(String)

The ND2 handler parses a large number of string key/value pairs,
converting values into numbers in many cases. But in almost every case,
no error handling was done in case the input value was not a number.

This change adds a blanket try/catch for NumberFormatException around
areas that parse lots of numbers, emitting a warning for any
non-conforming key/value pairs. This allows parsing to continue,
hopefully resulting in a less fragile ND2 reader.

It also catches NumberFormatException separately within the nested
TextInfoItem parse loop, so that parsing of TextInfoItem entries fail
individually rather than all-or-nothing.

Note that this change is nearly all whitespace indentation; the easiest
way to view the "real" change is using "git show -b" on the commit.
@ctrueden
Copy link
Member Author

ctrueden commented Sep 9, 2013

Thanks @melissalinkert. The problem was that the TextInfoItem parsing loop had a try/catch internally for one case before, which allowed the TextInfoItem entries to continue being parsed in certain cases. I amended the commit to add this back again, but in a more general way. Relevant new info from the commit message:

It also catches NumberFormatException separately within the nested
TextInfoItem parse loop, so that parsing of TextInfoItem entries fail
individually rather than all-or-nothing.

All the tests now pass on my system for nd2/jonas. Could you please include again to retest the whole enchilada?

@joshmoore
Copy link
Member

exclude removed.

@joshmoore
Copy link
Member

Builds as of today looked good, @ctrueden. @melissalinkert ?

@ctrueden
Copy link
Member Author

Thanks @joshmoore. 😂

melissalinkert added a commit that referenced this pull request Sep 11, 2013
@melissalinkert melissalinkert merged commit 6e5b7d4 into ome:dev_4_4 Sep 11, 2013
@melissalinkert
Copy link
Member

--rebased-to #700

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants