Consider using immutables for core classes

[julesjacobsen]

http://immutables.github.io/

import org.immutables.value.Value;
// Define abstract value type
@Value.Immutable
public interface ValueObject {
  String name();
  List<Integer> counts();
  Optional<String> description();
}

// Use generated immutable implementation
ValueObject valueObject =
    ImmutableValueObject.builder()
        .name("My value")
        .addCounts(1)
        .addCounts(2)
        .build();

// Or you can configure different @Value.Style
@Value.Immutable
abstract class AbstractItem {
  abstract String getName();
  abstract Set<String> getTags();
  abstract Optional<String> getDescription();
}

// Use generated value object
Item namelessItem = Item.builder()
    .setName("Nameless")
    .addTags("important", "relevant")
    .setDescription("Description provided")
    .build();

Item namedValue = namelessItem.withName("Named");

[julesjacobsen]

@cmungall I've converted the model classes and removed about 500 lines of boilerplate! However, all is not 100% ok. There are issues where previously the library was happy to tolerate null references, whereas immutables doesn't tolerate these by default.

For example in org.geneontology.obographs.model.meta.PropertyValue

    /**
     * Predicates correspond to OWL properties. Like all preds in this datamodel,
     * a pred is represented as a String which denotes a CURIE
     * 
     * @return the pred
     */
    public String getPred();

Implementing this in XrefPropertyValue, what should the default return value be if not set, as in the XrefPropertyValueTest

    public static XrefPropertyValue build() {
        return new XrefPropertyValue.Builder()
                .val(val)
                .lbl(lbl)
                .build();
    }

Is this a problem with the test? Should the value default to an empty String or some other value? Returning nulls are generally an unpleasant thing to do to users of a library.

[julesjacobsen]

@cmungall One other issue is there is a cyclical dependency between Meta and practically everything else. For example, in MetaTest

public static Meta build() {

        SynonymPropertyValue s = SynonymPropertyValueTest.build();
        // Can't build XrefPropertyValue due to missing Meta and pred, 
        //   but I'm trying to build a Meta using the XrefPropertyValue.  
        XrefPropertyValue xref = XrefPropertyValueTest.build(); 
        DefinitionPropertyValue def = new DefinitionPropertyValue.
                Builder().
                val(defval).
                xrefs(Arrays.asList(defXrefs)).
                build();

        return new Meta.Builder().
                definition(def).
                synonyms(Collections.singletonList(s)).
                xrefs(Collections.singletonList(xref)).
                subsets(Arrays.asList(subsets))
                .build();
    }

I get an error as the requisite Meta field of XrefPropertyValue is not set.

java.lang.IllegalStateException: Cannot build XrefPropertyValue, some of required attributes are not set [pred, meta]

Should Meta really be optional in most cases and would it be OK to have a Meta.empty() to replace a null?

[julesjacobsen]

Currently working in immutables branch

[julesjacobsen]

Looks really, really nice now. Here is a Node:

@JsonSerialize(as = Node.class)
@JsonDeserialize(as = Node.class)
@Value.Immutable
public abstract class AbstractNode implements NodeOrEdge {

    public enum RDFTYPES { CLASS, INDIVIDUAL, PROPERTY };

    @JsonProperty
    public abstract String getId();

    @JsonProperty("lbl")
    public abstract String getLabel();

    @JsonProperty
    @Nullable
    public abstract RDFTYPES getType();

}

Codes exactly as before, so need need for an API change.

[julesjacobsen]

@cmungall I think I have this working now and @matentzn has volunteered to kick the tyres on all his ontologies. Assuming this all works without issue, there are still two things to address.

1 - Can you recommend a specific order for the field names in all the model classes? 2 - In the SynonymPropertyValue there is a bit of fiddling with a SCOPES enum and some string representation which seems a bit awkward as the input value is not the same as the output.

[cmungall]

this is brilliant, thanks so much!

order: let's be consistent with https://owlcollab.github.io/oboformat/doc/GO.format.obo-1_4.html#S.3.5

so it would be

• id
• lbl
• type
• meta

for nodes

within meta

• def
• syn
• xref
• generic

sorry have not had a chance to look into SCOPES

[julesjacobsen]

OK, cheers. given Meta is common to all I'll make this the last element in all cases.

Meta is ordered:

@JsonPropertyOrder({
    "definition", 
    "comments", 
    "subsets", 
    "synonyms", 
    "xrefs", 
    "basicPropertyValues", 
    "version", 
    "deprecated"
})

I fixed the order of Edge to be:

@JsonPropertyOrder({
    "sub", 
    "pred", 
    "obj", 
    "meta"
})

[julesjacobsen]

@cmungall I'm getting back into this having lost the will to live getting the output to be 100% reproducible when trying to fix the ordering.

For the purposes of maximum utility for @matentzn the absolute order isn't important, but consistent order is. Plus at the end of the day what is more important - having a consistent data structure in memory for use or laid out for human inspection? I realise it sounds like defeat (it is) and I'm not happy as the output is now an artefact of my inability to tame the framework, but it was looking unlikely that having both consistency and order wasn't going to happen. Given we have neither with the current setup, can I suggest we take the easiest path and simply go with consistency?

[matentzn]

FWIW, I think that getting a FIXED order is 100 times more important than a PRESCRIBED one. So my vote is that we should move on that one before anything else.

[matentzn]

@balhoff you can chime in as well if you like!

[julesjacobsen]

New update - we now have cake and are eating it by the fistfull. Preliminary tests have showed we can have a nicely ordered and sorted output. The reason for this is obscure, but is based on the Set implementation used in OWLAPI and the need for a special sort method to be used on them. Thanks to @matentzn for pointing me in the right direction on this. Will tidy and submit a PR in a bit.

[julesjacobsen]

It's working- 3ddcee2e37f03a4886ae94738f9084f2472c9508

@matentzn should be testing this in Robot. n.b. Updating Robot to use the new Immutables branch. Once you're happy, Nico I'll create a PR. @balhoff can you give this a spin too?

[julesjacobsen]

Closing this now as the Immutables representation is used in the 0.3.0 release.