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.

Sign up for GitHub

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

Jump to bottom

REFACT - PDF Module error messages #347

Merged
merged 16 commits into from
May 21, 2018
Merged

REFACT - PDF Module error messages #347

merged 16 commits into from
May 21, 2018

Conversation

carlwilson
Copy link
Member

  • added markers for all error numbers;
  • reviewed and documented errors (in spreadsheet for wiki);
  • added a few new constants in place of magic numbers;
  • refactoring and simplification for:
    • Destinations
    • DocNode
    • FileSpecification
    • PdfModule
    • PageObject
    • PdfStream
  • removed unthrown exceptions in method definitions;
  • removed unused variables and code;
  • changed some private method names to be more descriptive of function;
  • explicit types for generic declarations;
  • removed associated and unnecessary casts; and
  • added missing @Override annotations.

carlwilson added 12 commits May 3, 2018 18:28
@carlwilson
REFACT - PDF Module error messages
7a507ad 
- added ID location markers for all PDF module error messages;
- moved messages exclusively for logger use to a different class;
@carlwilson
- factored out log message string constants;
60af3ee 
- corrected message typos and improved consistency of terms;
- corrected constant name typos;
@carlwilson
- replaced some magic numbers with named constants;
1600566 
- tidied use of generics and construction for `PdfDictionary`;
@carlwilson
- Destinations: added extra handler and message message to handle `…
d1aeef3 
…IOException` in constructor;

- `Destinations`: tidied instance construction;
- `Destinations`: factored out indirect object resolution;
@carlwilson
- DocNode: defensive check for null dictionary on construction;
2fb8e69 
- `DocNode`: removed debug code;
- `DocNode`: `getMediaBox()` now returns null when no meida box is found as documented;
@carlwilson
- Destination : seperated destination type and resolution typing ex…
f3c643d 
…ceptions;
@carlwilson
Merge branch 'integration' into refact/pdf-module-messages
6619603
@carlwilson
MAINT - Addressed Java warnings
105a546 
- explicit types for generic declarations;
- removed associated and unnecessary casts; and
- added missing `@Override` annotations.
@carlwilson
MAINT - Error constants and warnings
ff13be5 
- added markers for all error numbers;
- reviewed and documented errors (in spreadsheet for wiki);
- removed unthrown exceptions in method definitions;
- removed unused variables and code;
- changed some private method names to be more descriptive of function;
- explicit types for generic declarations;
- removed associated and unnecessary casts; and
- added missing `@Override` annotations.
@carlwilson
MAINT - Error constants and warnings
e66a1b6 
- added markers for all error numbers;
- factored out some repetetive patterns;
- removed unused variables and code;
- changed some private method names to be more descriptive of function;
- explicit types for generic declarations;
- removed associated and unnecessary casts; and
- added missing `@Override` annotations.
@carlwilson
MAINT - Error constants and warnings
b634284 
- added markers for all error numbers;
- simplified file specification handling;
- removed unused variables and code;
- explicit types for generic declarations; and
- removed associated and unnecessary casts.
@carlwilson
MAINT - Error constants and warnings
4aec15d 
- added markers for all error numbers;
- `int` constants for recursion limits;
- factored out object stream handling;
- small simplification to page label sequence handling;
- removed unthrown exceptions, unused variables and unused code;
- explicit types for generic declarations; and
- removed associated and unnecessary casts.
@codecov
Copy link

codecov bot commented May 17, 2018 •
edited
Loading

Codecov Report

Merging #347 into integration will increase coverage by <.01%.
The diff coverage is 53.27%.

Impacted file tree graph

@@                Coverage Diff                @@
##             integration     #347      +/-   ##
=================================================
+ Coverage          44.48%   44.49%   +<.01%     
- Complexity          3415     3418       +3     
=================================================
  Files                368      368              
  Lines              30321    30269      -52     
  Branches            5987     5979       -8     
=================================================
- Hits               13489    13468      -21     
+ Misses             14423    14395      -28     
+ Partials            2409     2406       -3
Impacted Files Coverage Δ Complexity Δ
...ava/edu/harvard/hul/ois/jhove/module/pdf/Name.java 66.66% <ø> (ø) 2 <0> (ø) ⬇️
...harvard/hul/ois/jhove/module/pdf/PdfXMPSource.java 46.66% <ø> (ø) 2 <0> (ø) ⬇️
...u/harvard/hul/ois/jhove/module/pdf/X1aProfile.java 71.42% <ø> (ø) 4 <0> (ø) ⬇️
.../hul/ois/jhove/module/pdf/PdfInvalidException.java 75% <ø> (ø) 3 <0> (ø) ⬇️
...va/edu/harvard/hul/ois/jhove/module/pdf/State.java 94.73% <ø> (ø) 2 <0> (ø) ⬇️
...rvard/hul/ois/jhove/module/pdf/PdfIndirectObj.java 44.44% <ø> (ø) 1 <0> (ø) ⬇️
...ard/hul/ois/jhove/module/pdf/MessageConstants.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...rd/hul/ois/jhove/module/pdf/StreamInputStream.java 100% <ø> (ø) 3 <0> (ø) ⬇️
.../hul/ois/jhove/module/pdf/PdfFlateInputStream.java 60.15% <ø> (+0.61%) 27 <0> (ø) ⬇️
...rvard/hul/ois/jhove/module/pdf/AProfileLevelA.java 100% <ø> (ø) 3 <0> (ø) ⬇️
... and 30 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc6037c...62b20be. Read the comment docs.

david-russo
david-russo reviewed May 18, 2018
View reviewed changes
jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java Outdated
@@ -1550,7 +1533,7 @@ else if ("f".equals(keyval)) {
}
else {
throw new PdfMalformedException
(MessageConstants.ERR_XREF_TABLE_OPERATOR_ILLEGAL,
(MessageConstants.ERR_XREF_TABLE_OPERATOR_ILLEGAL, // PDF-HUL-83
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF-HUL-84

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java Outdated
@@ -831,7 +815,7 @@ public final void parse(RandomAccessFile raf, RepInfo info)
}
// Beware infinite loop on badly broken file
if (_startxref == _prevxref) {
info.setMessage(new ErrorMessage(MessageConstants.ERR_XREF_TABLES_BROKEN,
info.setMessage(new ErrorMessage(MessageConstants.ERR_XREF_TABLES_BROKEN, // {PDF-HUL-134
Copy link
Member

@david-russo david-russo May 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra "{"

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java Outdated
MessageConstants.ERR_DOC_CAT_NOT_SIMPLE)) {
MessageConstants.ERR_DOC_CAT_TYPE_INVALID, // PDF-HUL-141
MessageConstants.ERR_DOC_CAT_NO_TYPE, // PDF-HUL-142
MessageConstants.ERR_DOC_CAT_NOT_SIMPLE)) { // PDF-HUL-142
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF-HUL-143?

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java Outdated
@@ -3021,7 +2962,7 @@ protected Property buildPageProperty(PageObject page,
// but we do report that we don't report them.
if (!_skippedAnnotationsReported) {
info.setMessage(new InfoMessage
(annotationsSkippedString));
(MessageConstants.INF_ANNOTATIONS_SKIPPED));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF-HUL-115

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/PdfModule.java Outdated
@@ -2943,7 +2884,7 @@ protected void addPagesProperty(List<Property> metadataList, RepInfo info)
else {
if (!_skippedPagesReported) {
info.setMessage(new InfoMessage
(pagesSkippedString));
(MessageConstants.INF_PAGES_SKIPPED));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF-HUL-112

jhove-modules/src/main/java/edu/harvard/hul/ois/jhove/module/pdf/Parser.java Outdated
@@ -96,7 +94,7 @@ public Token getNext (long max) throws IOException, PdfException
else if (tok instanceof DictionaryEnd) {
--_dictDepth;
if (_dictDepth < 0) {
throw new PdfMalformedException (MessageConstants.ERR_DICT_DELIMETERS_IMPROPERLY_NESTED);
throw new PdfMalformedException (MessageConstants.ERR_DICT_DELIMITERS_IMPROPERLY_NESTED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PDF-HUL-33

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, am thinking of adding a consistency check for errors in unit tests, but this is why adding an error number to the message soon is a good idea. Then the compiler takes care of it.

@carlwilson carlwilson merged commit 667fe96 into integration May 21, 2018
@carlwilson carlwilson deleted the refact/pdf-module-messages branch May 21, 2018 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@david-russo david-russo david-russo left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carlwilson @david-russo