Errors in Types cause NPE when printing stack trace

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: None
  • Fix Version/s: None
  • Component/s: main
  • Labels:
    None

Description

On trunk:

org.geotools.feature.types.Types reports some errors by throwing the deprecated exception org.geotools.feature.IllegalAttributeException, which should never be used because it puts org.opengis.feature.IllegalAttributeException into an invalid state (descriptor = null) that causes NullPointerException in toString(). This make debugging hugely painful because toString() is called when constructing the stack trace, and the NPE causes the original exception and trace to be lost.

Please refactor org.geotools.feature.types.Types to not use org.geotools.feature.IllegalAttributeException.

Issue Links

is related to

Bug - A problem which impairs or prevents the functions of the product. GEO-156 NPE in org.opengis.feature.IllegalAttributeException.toString()

  • Major - Major loss of function.
  • Closed - The issue is considered finished, the resolution is correct. Issues which are not closed can be reopened.

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
Andrea Aime added a comment -
Hmmm... we should probably have deprecated Types to start with. If you need to build complex types you should build a FeatureTypeBuilder and a FeatureBuilder mimicking the SimpleFeatureTypeBuilder and the SimpleFeatureBuilder. They aren't there simply for the reason we have no usage for complex features at the moment, but that should be the way to go.
Show
Andrea Aime added a comment - Hmmm... we should probably have deprecated Types to start with. If you need to build complex types you should build a FeatureTypeBuilder and a FeatureBuilder mimicking the SimpleFeatureTypeBuilder and the SimpleFeatureBuilder. They aren't there simply for the reason we have no usage for complex features at the moment, but that should be the way to go.
Hide
Permalink
Ben Caradoc-Davies added a comment -
This is not a complex features problem. The broken use case is calling the constructor for gt-main AttributeImpl for a non-nillable type with null content. The constructor for AttributeImpl calls Types.validate, and org.geotools.feature.IllegalAttributeException is thrown. When the stack trace is printed, because the org.geotools.feature.IllegalAttributeException initialises its super org.opengis.feature.IllegalAttributeException with a null descriptor, toString throws a NullPointerException, and the useful diagnostic information is lost.

Expected outcome: stack trace contains useful information including the name of the non-nillable type and the reason for the error.
Actual outcome: trashed stack trace, and NullPointerException with no message.
Show
Ben Caradoc-Davies added a comment - This is not a complex features problem. The broken use case is calling the constructor for gt-main AttributeImpl for a non-nillable type with null content. The constructor for AttributeImpl calls Types.validate, and org.geotools.feature.IllegalAttributeException is thrown. When the stack trace is printed, because the org.geotools.feature.IllegalAttributeException initialises its super org.opengis.feature.IllegalAttributeException with a null descriptor, toString throws a NullPointerException, and the useful diagnostic information is lost. Expected outcome: stack trace contains useful information including the name of the non-nillable type and the reason for the error. Actual outcome: trashed stack trace, and NullPointerException with no message.
Hide
Permalink
Ben Caradoc-Davies added a comment -
An alternative to fixing this issue would be to add a null pointer check in org.opengis.feature.IllegalAttributeException, as described in GEO-156.
Show
Ben Caradoc-Davies added a comment - An alternative to fixing this issue would be to add a null pointer check in org.opengis.feature.IllegalAttributeException, as described in GEO-156.
Hide
Permalink
Ben Caradoc-Davies added a comment - - edited
This bug makes debugging GeoServer app-schema mapping files nearly impossible, because any bad mapping causes feature construction to fail Types.validate, but the user gets a useless NPE!
Show
Ben Caradoc-Davies added a comment - - edited This bug makes debugging GeoServer app-schema mapping files nearly impossible, because any bad mapping causes feature construction to fail Types.validate, but the user gets a useless NPE!
Hide
Permalink
Ben Caradoc-Davies added a comment -
I have a patch for this bug.
Show
Ben Caradoc-Davies added a comment - I have a patch for this bug.
Hide
Permalink
Ben Caradoc-Davies added a comment -
Increased priority to Major, as it is a Major PITA when debugging GeoServer configurations.
Show
Ben Caradoc-Davies added a comment - Increased priority to Major, as it is a Major PITA when debugging GeoServer configurations.
Hide
Permalink
Ben Caradoc-Davies added a comment -
Patch attached.
Show
Ben Caradoc-Davies added a comment - Patch attached.
Hide
Permalink
Andrea Aime added a comment -
The patch looks sane to me. If no one else cares to review it I'd say you just go ahead and commit it.
Show
Andrea Aime added a comment - The patch looks sane to me. If no one else cares to review it I'd say you just go ahead and commit it.
Hide
Permalink
Jody Garnett added a comment -
I applied your patch; but also fixed the toString implementation in geoapi.
Show
Jody Garnett added a comment - I applied your patch; but also fixed the toString implementation in geoapi.
Hide
Permalink
Ben Caradoc-Davies added a comment -
Thanks very much. This change is sure to ease debugging and configuration troubleshooting.
Show
Ben Caradoc-Davies added a comment - Thanks very much. This change is sure to ease debugging and configuration troubleshooting.
Hide
Permalink
Jody Garnett added a comment -
Remember Ben we can go through these things faster if you want to arrange a Skype or IRC review (you can commit and we can go over what was done together). Waiting for someone to apply your patch in each case is going to be stressful and slow you down; for many things like this we should be able to go over what is needed on the development list and give you the go ahead for obvious mistakes like this one.

Jody
Show
Jody Garnett added a comment - Remember Ben we can go through these things faster if you want to arrange a Skype or IRC review (you can commit and we can go over what was done together). Waiting for someone to apply your patch in each case is going to be stressful and slow you down; for many things like this we should be able to go over what is needed on the development list and give you the go ahead for obvious mistakes like this one. Jody
Hide
Permalink
Andrea Aime added a comment -
Mass closing all issues that have been in "resolved" state for 2 months or more without any feedback or update
Show
Andrea Aime added a comment - Mass closing all issues that have been in "resolved" state for 2 months or more without any feedback or update

People

  • Assignee:
    Jody Garnett
    Reporter:
    Ben Caradoc-Davies
Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: