frizbog / gedcom4j

Java library for reading/writing genealogy files in GEDCOM format
http://gedcom4j.org
53 stars 36 forks source link

null pointer error in Individual.getAttributesOf Type #189

Open gjwo opened 6 years ago

gjwo commented 6 years ago

Can't see what's going wrong here, I get this error on some individuals (presumably those with no attributes) it works OK for others. is there something I should check before the call?

865 [Thread-0] INFO org.eclipse.jetty.server.Server - Started @1698ms 4079 [qtp2104434427-16] ERROR spark.http.matching.GeneralError - java.lang.NullPointerException at org.gedcom4j.model.Individual.getAttributesOfType(Individual.java:586)

line appears to be  IndividualAttribute ir = (IndividualAttribute)var3.next(); see below

at me.gjwo.gedcom.FactPicker.getIndAttributeTableData(FactPicker.java:111) at me.gjwo.gedcom.pages.elements.PersonAttributesElement.render(PersonAttributesElement.java:36) at me.gjwo.gedcom.pages.IndividualPage.render(IndividualPage.java:44) at me.gjwo.gedcom.GedcomView.lambda$main$2(GedcomView.java:46) at spark.RouteImpl$1.handle(RouteImpl.java:72)

    public List<IndividualAttribute> getAttributesOfType(IndividualAttributeType type) {
        List<IndividualAttribute> result = new ArrayList(0);
        Iterator var3 = this.attributes.iterator();

        while(var3.hasNext()) {
           IndividualAttribute ir = (IndividualAttribute)var3.next();
            if (ir.getType() == type) {
                result.add(ir);
            }
        }

        return result;
    }
frizbog commented 6 years ago

I think your attributes collection in your FactPicker instance has a null in it. You should test that ir is non-null before doing getType() on it.

On Fri, Apr 13, 2018, 12:19 PM Graham Wood notifications@github.com wrote:

Can't see what's going wrong here, I get this error on some individuals (presumably those with no attributes) it works OK for others. is there something I should check before the call?

865 [Thread-0] INFO org.eclipse.jetty.server.Server - Started @1698ms 4079 [qtp2104434427-16] ERROR spark.http.matching.GeneralError - java.lang.NullPointerException at org.gedcom4j.model.Individual.getAttributesOfType(Individual.java:586)

line appears to be IndividualAttribute ir = (IndividualAttribute)var3.next(); see below

at me.gjwo.gedcom.FactPicker.getIndAttributeTableData(FactPicker.java:111) at me.gjwo.gedcom.pages.elements.PersonAttributesElement.render(PersonAttributesElement.java:36) at me.gjwo.gedcom.pages.IndividualPage.render(IndividualPage.java:44) at me.gjwo.gedcom.GedcomView.lambda$main$2(GedcomView.java:46) at spark.RouteImpl$1.handle(RouteImpl.java:72)

public List<IndividualAttribute> getAttributesOfType(IndividualAttributeType type) {
    List<IndividualAttribute> result = new ArrayList(0);
    Iterator var3 = this.attributes.iterator();

    while(var3.hasNext()) {
       IndividualAttribute ir = (IndividualAttribute)var3.next();
        if (ir.getType() == type) {
            result.add(ir);
        }
    }

    return result;
}

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/frizbog/gedcom4j/issues/189, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxHYcWTs6-8XHHPEyZO9nswhUCW9E-Tks5toNAQgaJpZM4TTuKy .

gjwo commented 6 years ago

I'm not seeing it, do you mean in the attribute type or the IndividualAttribute? I am only supplying literal AttributeTypes in order to retrieve attributes using your library see below:

  /**
     *  pickIndEventTableData    -  extracts individual event data as string lists
     * @param eventTypes            List of events types to be included in table data
     * @return                      A list of label rows each of which is a list of strings
     */
    public List <List<String>> getIndAttributeTableData(List <IndividualAttributeType> eventTypes)
    {
        List <List <String>> results = new ArrayList<>();
        for (IndividualAttributeType type:eventTypes)
            for (IndividualAttribute attribute : person.getAttributesOfType(type))
                results.add(pickAttributeRow(attribute, true));
        return results;
    }

called from

    public String render()
    {
        PersonFactBuilder fb = new PersonFactBuilder(person);
        return HtmlWrapper.wrapTable(
                factPicker.getIndAttributeTableData(
                        List.of(IndividualAttributeType.OCCUPATION,
                                IndividualAttributeType.RESIDENCE)),
                        List.of("Attribute","Date", "Place","Description"));
    }
frizbog commented 6 years ago

I'm saying that your person object has a collection of IndividualAttribute object references, and one of the references in the collection is null. When the iterator goes through the collection, the next() function returns a null at some point, and the attempt to access a function on that null object reference is throwing the NPE.

On Fri, Apr 13, 2018, 5:20 PM Graham Wood notifications@github.com wrote:

I'm not seeing it, do you mean in the attribute type or the IndividualAttribute? I am only supplying literal AttributeTypes in order to retrieve attributes using your library see below:

/**

  • pickIndEventTableData - extracts individual event data as string lists
  • @param eventTypes List of events types to be included in table data
  • @return A list of label rows each of which is a list of strings */ public List <List> getIndAttributeTableData(List eventTypes) { List <List > results = new ArrayList<>(); for (IndividualAttributeType type:eventTypes) for (IndividualAttribute attribute : person.getAttributesOfType(type)) results.add(pickAttributeRow(attribute, true)); return results; }

called from

public String render()
{
    PersonFactBuilder fb = new PersonFactBuilder(person);
    return HtmlWrapper.wrapTable(
            factPicker.getIndAttributeTableData(
                    List.of(IndividualAttributeType.OCCUPATION,
                            IndividualAttributeType.RESIDENCE)),
                    List.of("Attribute","Date", "Place","Description"));
}

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frizbog/gedcom4j/issues/189#issuecomment-381263433, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxHYZvTdRpj9Uxj_kZVg5gEjw8y9Bo_ks5toRaUgaJpZM4TTuKy .

gjwo commented 6 years ago

OK I think the whole collection is null, which I have now tested for to avoid crashing. This is a good library but It could be much better generally if you could enhance it to initialise your data structures to the appropriate empty Collections, rather than null, if you find no data for a collection in the .ged file. This would avoid all sorts of issues and for each loops would work as intended.

frizbog commented 6 years ago

This was a deliberate design decision to reduce memory footprint especially on mobile devices that have very strict limits on heap size. The library actually used to do what you suggest and it typically wasted about 60% of the heap utilization on empty collections. The getters for collections on all objects are overloaded so if you say object.getCollection(true), it will initialize the collection to an empty list if needed before returning.

On Sat, Apr 14, 2018, 5:12 AM Graham Wood notifications@github.com wrote:

OK I think the whole collection is null, which I have now tested for now to avoid crashing. It would be much better generally of you could enhance your library to initialise your data structures to the appropriate empty Collections rather than null if you find no data for a collection in the .ged file. This would avoid all sorts of issues and for each loops would work as intended.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frizbog/gedcom4j/issues/189#issuecomment-381315567, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxHYVktkWBmmEnThs4NwEM2Rmv0Sr_Eks5tob2RgaJpZM4TTuKy .

gjwo commented 6 years ago

OK that makes sense. Does this work for collections and selections from collections as in the following examples:

frizbog commented 6 years ago

Yes it does!

On Sat, Apr 14, 2018, 5:22 PM Graham Wood notifications@github.com wrote:

OK that makes sense. Does this work for collections and selections from collections as in the following examples:

  • individual.getEvents(true);
  • individual.getEventsOfType(IndividualEventType.CENSUS,true);
  • individual.getAttributesOfType(idividualAttributeType.OCCUPATION,true);

  • etc

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frizbog/gedcom4j/issues/189#issuecomment-381361465, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxHYQwe6VJ-VlTccHCzyN9iHyGXmdVrks5tomisgaJpZM4TTuKy .

gjwo commented 6 years ago

Excellent!

gjwo commented 6 years ago

This seems to work for getEvents(true) of both types, but not for getEventsOfType(... which throws a compile error.

frizbog commented 6 years ago

Ok looks like I missed one. It's the getters and setters of the model properties that do that optional initialization.

On Sun, Apr 15, 2018, 4:32 PM Graham Wood notifications@github.com wrote:

This seems to work for getEvents(true) of both types, but not for getEventsOfType(... which throws a compile error.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/frizbog/gedcom4j/issues/189#issuecomment-381435741, or mute the thread https://github.com/notifications/unsubscribe-auth/ACxHYQJp97RKR-3xKDqc1Agay5vG5oZXks5to65CgaJpZM4TTuKy .

gjwo commented 6 years ago

It is the "getEventsOfType" calls that I use the most (as caused the null pointer problem at the top of this thread) I wouldn't normally use the "getEvent" first I think the only time was when I implemented the missing Family,getEventsOfType method. So I would say the "getEventsOfType" most need to be initialised with empty sets if required.