Fix EnumTypedef name problem (#1091)

* ckp

* Fix EnumTypedef name problem

## Description of Changes
re: Issue #126

The code for handling enum types in Group.java is
incorrect. When creating a new enum type, it is appropriate to
search only the current group for a conflicting name and this is
what the current code does.  But when an enum typed variable
searches for the matching enum type, it must search not only the
current group but all parent groups and the current code does
not do that.

The fix consists of two similar parts.
1. Modify Group.findEnumeration to take an extra boolean parameter
to control if search is this group only or to search up the group's parents.
2. Modify Group.Builder.findEnumTypedef to act like \#1 but to search
the sequence of parent Group.Builder instances.

As a consequence, this PR modifies a number of other files to
adapt to the modified signatures.

* 1. Remove unused import in H5HeaderNew
2. Add overloaded functions to Group.java to restore
   access to original versions of findEnumTypedef and findEnumeration.

* Force re-test

* It turns out that I missed the error in the code in H5headerNew that
attempts to match an enum typed variable with the proper enumeration
type declaration.

The problem and fix (as described by a comment in H5headerNew)
is a bit of a hack. It should be fixed in H5Object.read().
Unfortunately, information is not being passed down so that
the proper fix can be applied early in the HDF5->CDM translation.
Fixing this would affect a lot of function signatures.

Also modified TestEnumTypedef.java to test two cases:
1. the actual enum type def is in the same group as the variable that uses it.
2. the actual enum type def is in a parent group of the variable that uses it

## Misc. Other Changes
* Suppress printing of _NCProperties to simplify text-based comparison testing.

* ## Addendum 2

Sigh! Apparently NetcdfFile.java defaulted to using H5iosp instead
of H5iospNew. This meant that many of my changes were being bypassed.

So, modify NetcdfFile to default to H5iospNew.

* Undo change to NetcdfFile.java'

* test4

* NCProperties fix

* ## Additional modifications
* NetcdfFile.java: convert to use H5iospNew (needed by TestH5iosp.java)
* H5headerNew.java: provide get function for accessing the btree (needed by TestDataBTree.java)
* H5iospNew.java: make getRandomAccessFile() method public (needed by tests)
* CompareNetcdf2.java: Add a constructor to specify if attribute name comparison should ignore case or not. It turns out that some tests require case sensitive name matching. Specifically TestCoordSysCompare.java and TestN3iospCompare.java

* Apply Spotless

* remove debugging

---------

Co-authored-by: haileyajohnson <hailey.johnson@ufl.edu>

bufr/src/main/java/ucar/nc2/iosp/bufr/Construct2.java

@@ -299,7 +299,7 @@ else if (nbytes == 4)
       Group g = struct.getParentGroupOrRoot();
       if (g == null)
         log.warn("Struct parent group is null.");
-      EnumTypedef enumTypedef = g.findEnumeration(ct.getName());
+      EnumTypedef enumTypedef = g.findEnumeration(ct.getName(), false);
       if (enumTypedef == null) {
         enumTypedef = new EnumTypedef(ct.getName(), ct.getMap());
         g.addEnumeration(enumTypedef);

cdm/core/src/main/java/ucar/nc2/Attribute.java

@@ -38,7 +38,7 @@ public class Attribute extends CDMNode {
 
   /** @deprecated move to jni.Nc4Iosp */
   @Deprecated
-  static final String[] SPECIALS =
+  static public final String[] SPECIALS =
       {CDM.NCPROPERTIES, CDM.ISNETCDF4, CDM.SUPERBLOCKVERSION, CDM.DAP4_LITTLE_ENDIAN, CDM.EDU_UCAR_PREFIX};
 
   /**

cdm/core/src/main/java/ucar/nc2/Group.java

@@ -2,8 +2,10 @@
  * Copyright (c) 1998-2018 University Corporation for Atmospheric Research/Unidata
  * See LICENSE for license information.
  */
+
 package ucar.nc2;
 
+import static java.util.Optional.ofNullable;
 import static ucar.nc2.NetcdfFiles.reservedFullName;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
@@ -295,7 +297,7 @@ public Dimension findDimensionLocal(String shortName) {
 
   /** The attributes contained by this Group. */
   public AttributeContainer attributes() {
-    return attributes;
+    return AttributeContainer.filter(attributes, Attribute.SPECIALS);
   }
 
   /** Find the attribute by name, return null if not exist */
@@ -374,20 +376,29 @@ public boolean removeAttributeIgnoreCase(String attName) {
 
   ////////////////////////////////////////////////////////////////////////
 
-  /** Find a Enumeration in this or a parent Group, using its short name. */
+  /** Find a Enumeration in this Group, using its short name. */
   @Nullable
   public EnumTypedef findEnumeration(String name) {
+    // Keep the old behavior
+    return findEnumeration(name, false);
+  }
+
+  /** Find a Enumeration in this or optionally the parent Groups, using its short name. */
+  @Nullable
+  public EnumTypedef findEnumeration(String name, boolean searchup) {
     if (name == null)
       return null;
     // name = NetcdfFile.makeNameUnescaped(name);
-    for (EnumTypedef d : enumTypedefs) {
+    // search this group's EnumTypedefs
+    for (EnumTypedef d : this.getEnumTypedefs()) {
       if (name.equals(d.getShortName()))
         return d;
     }
-    Group parent = getParentGroup();
-    if (parent != null)
-      return parent.findEnumeration(name);
-
+    if (searchup) {
+      Group parent = getParentGroup();
+      if (parent != null)
+        return parent.findEnumeration(name, searchup);
+    }
     return null;
   }
 
@@ -1114,6 +1125,52 @@ public Group.Builder commonParent(Group.Builder other) {
       return other;
     }
 
+    public Optional<EnumTypedef> findSimilarEnumTypedef(EnumTypedef template, boolean searchup, boolean anyname) {
+      Optional<EnumTypedef> ed = Optional.empty();
+      assert (template != null);
+      // search this group builders's EnumTypedefs but with constraint on name
+      ed = this.enumTypedefs.stream()
+          .filter(e -> (anyname || !template.getShortName().equals(e.getShortName())) && e.equalsMapOnly(template))
+          .findFirst();
+      if (ed.isPresent())
+        return ed;
+      // Optionally search parents
+      if (searchup) {
+        Group.Builder gb = getParentGroup();
+        if (gb != null)
+          ed = gb.findSimilarEnumTypedef(template, searchup, anyname);
+        if (ed.isPresent())
+          return ed;
+      }
+      return Optional.empty();
+    }
+
+    /** Find a Enumeration in this Group Builder, using its short name. */
+    public Optional<EnumTypedef> findEnumTypedef(String name) {
+      // Keep the old behavior
+      return findEnumTypedef(name, false);
+    }
+
+    /** Find a Enumeration in this or a parent Group Builder, using its short name. */
+    public Optional<EnumTypedef> findEnumTypedef(String name, boolean searchup) {
+      if (name == null)
+        return Optional.empty();
+      // name = NetcdfFile.makeNameUnescaped(name);
+      // search this group builders's EnumTypedefs
+      Optional<EnumTypedef> ed = this.enumTypedefs.stream().filter(e -> e.shortName.equals(name)).findFirst();
+      if (ed.isPresent())
+        return ed;
+      // Optionally search parents
+      if (searchup) {
+        Group.Builder gb = getParentGroup();
+        if (gb != null)
+          ed = gb.findEnumTypedef(name, searchup);
+        if (ed.isPresent())
+          return ed;
+      }
+      return Optional.empty();
+    }
+
     public Builder addEnumTypedef(EnumTypedef typedef) {
       Preconditions.checkNotNull(typedef);
       enumTypedefs.add(typedef);
@@ -1131,7 +1188,7 @@ public Builder addEnumTypedefs(Collection<EnumTypedef> typedefs) {
      * Return new or existing.
      */
     public EnumTypedef findOrAddEnumTypedef(String name, Map<Integer, String> map) {
-      Optional<EnumTypedef> opt = findEnumTypedef(name);
+      Optional<EnumTypedef> opt = findEnumTypedef(name, false); // Find in this group
       if (opt.isPresent()) {
         return opt.get();
       } else {
@@ -1141,10 +1198,6 @@ public EnumTypedef findOrAddEnumTypedef(String name, Map<Integer, String> map) {
       }
     }
 
-    public Optional<EnumTypedef> findEnumTypedef(String name) {
-      return this.enumTypedefs.stream().filter(e -> e.shortName.equals(name)).findFirst();
-    }
-
     /** Add a Variable, throw error if one of the same name if it exists. */
     public Builder addVariable(Variable.Builder<?> variable) {
       Preconditions.checkNotNull(variable);

cdm/core/src/main/java/ucar/nc2/NetcdfFile.java

@@ -41,12 +41,12 @@
 import ucar.ma2.InvalidRangeException;
 import ucar.ma2.Section;
 import ucar.ma2.StructureDataIterator;
+import ucar.nc2.internal.iosp.hdf5.H5iospNew;
 import ucar.nc2.internal.iosp.netcdf3.N3headerNew;
 import ucar.nc2.iosp.AbstractIOServiceProvider;
 import ucar.nc2.iosp.IOServiceProvider;
 import ucar.nc2.iosp.IospHelper;
 import ucar.nc2.iosp.hdf5.H5header;
-import ucar.nc2.iosp.hdf5.H5iosp;
 import ucar.nc2.iosp.netcdf3.N3header;
 import ucar.nc2.iosp.netcdf3.N3iosp;
 import ucar.nc2.iosp.netcdf3.SPFactory;
@@ -830,7 +830,7 @@ private static IOServiceProvider getIosp(RandomAccessFile raf) throws IOExceptio
       return SPFactory.getServiceProvider();
 
     } else if (H5header.isValidFile(raf)) {
-      return new H5iosp();
+      return new H5iospNew();
 
     } else {
 

cdm/core/src/main/java/ucar/nc2/Variable.java

@@ -1830,7 +1830,7 @@ protected Variable(Builder<?> builder, Group parentGroup) {
     this.cache = builder.cache;
 
     if (this.dataType.isEnum()) {
-      this.enumTypedef = this.group.findEnumeration(builder.enumTypeName);
+      this.enumTypedef = this.group.findEnumeration(builder.enumTypeName, true);
       if (this.enumTypedef == null) {
         throw new IllegalStateException(
             String.format("EnumTypedef '%s' does not exist in a parent Group", builder.enumTypeName));

cdm/core/src/main/java/ucar/nc2/internal/iosp/hdf5/H5headerNew.java

@@ -17,13 +17,8 @@
 import java.nio.ShortBuffer;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
-import java.util.ArrayList;
-import java.util.Formatter;
-import java.util.HashMap;
-import java.util.Iterator;
-import java.util.List;
-import java.util.Map;
-import java.util.TimeZone;
+import java.util.*;
+
 import ucar.ma2.Array;
 import ucar.ma2.ArrayChar;
 import ucar.ma2.ArrayObject;
@@ -44,7 +39,6 @@
 import ucar.nc2.Structure;
 import ucar.nc2.Variable;
 import ucar.nc2.constants.CDM;
-import ucar.nc2.filter.Filter;
 import ucar.nc2.internal.iosp.hdf4.HdfEos;
 import ucar.nc2.internal.iosp.hdf4.HdfHeaderIF;
 import ucar.nc2.internal.iosp.hdf5.H5objects.DataObject;
@@ -498,7 +492,7 @@ private boolean makeNetcdfGroup(Group.Builder parentGroup, H5Group h5group) thro
         }
 
         if (facadeNested.dobj.mdt.map != null) {
-          EnumTypedef enumTypedef = parentGroup.findEnumTypedef(facadeNested.name).orElse(null);
+          EnumTypedef enumTypedef = parentGroup.findEnumTypedef(facadeNested.name, true).orElse(null);
           if (enumTypedef == null) {
             DataType basetype;
             switch (facadeNested.dobj.mdt.byteSize) {
@@ -554,7 +548,7 @@ private boolean makeNetcdfGroup(Group.Builder parentGroup, H5Group h5group) thro
             }
             // This code apparently addresses the possibility of an anonymous enum LOOK ??
             if (enumTypeName.isEmpty()) {
-              EnumTypedef enumTypedef = parentGroup.findEnumTypedef(facadeNested.name).orElse(null);
+              EnumTypedef enumTypedef = parentGroup.findEnumTypedef(facadeNested.name, true).get();
               if (enumTypedef == null) {
                 enumTypedef = new EnumTypedef(facadeNested.name, facadeNested.dobj.mdt.map);
                 parentGroup.addEnumTypedef(enumTypedef);
@@ -1553,14 +1547,51 @@ private boolean makeVariableShapeAndType(Group.Builder parent, Variable.Builder
 
     // set the enumTypedef
     if (dt.isEnum()) {
-      // TODO Not sure why, but there may be both a user type and a "local" mdt enum. May need to do a value match?
-      EnumTypedef enumTypedef = parent.findEnumTypedef(mdt.enumTypeName).orElse(null);
-      if (enumTypedef == null) { // if shared object, wont have a name, shared version gets added later
-        EnumTypedef local = new EnumTypedef(mdt.enumTypeName, mdt.map);
-        enumTypedef = parent.enumTypedefs.stream().filter((e) -> e.equalsMapOnly(local)).findFirst().orElse(local);
-        parent.addEnumTypedef(enumTypedef);
-      }
-      v.setEnumTypeName(enumTypedef.getShortName());
+      // dmh: HDF5, at least as used by netcdf-4, defines an enumeration type multiple times.
+      // One is when the enum type is defined, and then for every variable or field typed by that
+      // enum type, another enum type is created but with the name of the variable/field instead of
+      // the name of the actual type. This appears to be because H5header(New) does not pass
+      // down enough information for the HDF5->CDM translator to recognize the second case.
+      // The solution provided is to see if the following conditions hold when an enum typed
+      // variable is encountered:
+      // 1. The enum name is the same as the variable name.
+      // 2. There is no enum of that name in the current group.
+      // 3. There is another enum type in some containing group
+      // that is structurally identical to the EnumTypedef -- but with a
+      // different name -- associated with the variable. This is in contrast
+      // to testing for name equality.
+      //
+      // If all three conditions are true, then use the enum found
+      // in condition 3 as the enum type for the variable.
+      // Note that provision is made only for a shortname rather than a
+      // fully qualified name, which is incorrect, but too hard to change.
+      //
+      // If condition 1 if false, then it is ok to create a new EnumTypedef.
+      // Else if condition 2 is false then it is an error because the existence
+      // of such an enum is illegal netcdf-4 because duplicate name.
+      // Else if condition 3 is false, then it is an error because the
+      // variable's enum type is not defined.
+
+      EnumTypedef actualEnumTypedef = null; // The final chosen EnumTypedef
+      // Create the variables EnumTypedef on speculation
+      EnumTypedef template = new EnumTypedef(mdt.enumTypeName, mdt.map);
+      if (template.getShortName().equals(v.shortName)) { // Condition 1
+        Optional<EnumTypedef> candidate = parent.findEnumTypedef(mdt.enumTypeName, false);
+        if (!candidate.isPresent()) { // Condition 2
+          candidate = parent.findSimilarEnumTypedef(template, true, false);
+          if (candidate.isPresent()) // Condition 3; use correct EnumTypedef
+            actualEnumTypedef = candidate.get();
+          else { // !Condition 3
+            log.warn("EnumTypedef is missing for variable: {}", v.shortName);
+            throw new IllegalStateException("EnumTypedef is missing for variable: " + v.shortName);
+          }
+        } else { // !Condition 2
+          log.warn("Duplicate name: EnumTypedef and Variable: {}", v.shortName);
+          throw new IllegalStateException("Duplicate name: EnumTypedef and Variable: " + v.shortName);
+        }
+      } else // ! Condition 1; use template
+        actualEnumTypedef = template;
+      v.setEnumTypeName(actualEnumTypedef.getShortName());
     }
 
     return true;
@@ -1610,6 +1641,10 @@ public class Vinfo {
     boolean useFillValue;
     byte[] fillValue;
 
+    public DataBTree getBtree() {
+      return btree;
+    }
+
     public String getCompression() {
       if (mfp == null)
         return null;

cdm/core/src/main/java/ucar/nc2/internal/iosp/hdf5/H5iospNew.java

@@ -97,7 +97,7 @@ public String getFileTypeDescription() {
     return "Hierarchical Data Format, version 5";
   }
 
-  RandomAccessFile getRandomAccessFile() {
+  public RandomAccessFile getRandomAccessFile() {
     return raf;
   }
 

cdm/core/src/main/java/ucar/nc2/internal/iosp/hdf5/H5objects.java

@@ -1774,7 +1774,7 @@ String getName() {
     }
 
     int getId() {
-      return (int) this.properties.get(Filters.Keys.ID);
+      return ((Short) this.properties.get(Filters.Keys.ID)).intValue();
     }
 
     Map<String, Object> getProperties() {

cdm/core/src/main/java/ucar/nc2/iosp/hdf5/H5header.java

@@ -489,7 +489,7 @@ private boolean makeNetcdfGroup(ucar.nc2.Group ncGroup, H5Group h5group) throws
         }
 
         if (facadeNested.dobj.mdt.map != null) {
-          EnumTypedef enumTypedef = ncGroup.findEnumeration(facadeNested.name);
+          EnumTypedef enumTypedef = ncGroup.findEnumeration(facadeNested.name, false);
           if (enumTypedef == null) {
             DataType basetype;
             switch (facadeNested.dobj.mdt.byteSize) {
@@ -547,7 +547,7 @@ private boolean makeNetcdfGroup(ucar.nc2.Group ncGroup, H5Group h5group) throws
             // This code apparently addresses the possibility of an anonymous enum LOOK ??
             String ename = enumTypedef.getShortName();
             if (ename == null || ename.isEmpty()) {
-              enumTypedef = ncGroup.findEnumeration(facadeNested.name);
+              enumTypedef = ncGroup.findEnumeration(facadeNested.name, false);
               if (enumTypedef == null) {
                 enumTypedef = new EnumTypedef(facadeNested.name, facadeNested.dobj.mdt.map);
                 ncGroup.addEnumeration(enumTypedef);

cdm/core/src/main/java/ucar/nc2/ncml/NcMLReader.java

@@ -996,7 +996,7 @@ private void readVariable(NetcdfDataset ds, Group g, Group refg, Element varElem
     if (dtype.isEnum()) {
       String typedefS = varElem.getAttributeValue("typedef");
       if (typedefS != null)
-        typedef = g.findEnumeration(typedefS);
+        typedef = g.findEnumeration(typedefS, true);
     }
 
     String shape = varElem.getAttributeValue("shape");
@@ -1207,7 +1207,7 @@ private Variable readVariableNew(NetcdfDataset ds, Group g, Structure parentS, E
     if (dtype.isEnum()) {
       String typedefS = varElem.getAttributeValue("typedef");
       if (typedefS != null)
-        typedef = g.findEnumeration(typedefS);
+        typedef = g.findEnumeration(typedefS, true);
     }
 
     String shape = varElem.getAttributeValue("shape");

cdm/core/src/main/java/ucar/nc2/util/CompareNetcdf2.java

@@ -165,6 +165,7 @@ public static boolean checkContains(String what, List<Object> container, List<Ob
   private boolean showCompare;
   private boolean showEach;
   private boolean compareData;
+  private boolean ignoreattrcase;
 
   public CompareNetcdf2() {
     this(new Formatter(System.out));
@@ -175,10 +176,16 @@ public CompareNetcdf2(Formatter f) {
   }
 
   public CompareNetcdf2(Formatter f, boolean showCompare, boolean showEach, boolean compareData) {
+    this(f, showCompare, showEach, compareData, true);
+  }
+
+  public CompareNetcdf2(Formatter f, boolean showCompare, boolean showEach, boolean compareData,
+      boolean ignoreattrcase) {
     this.f = f;
     this.compareData = compareData;
     this.showCompare = showCompare;
     this.showEach = showEach;
+    this.ignoreattrcase = ignoreattrcase;
   }
 
   public boolean compare(NetcdfFile org, NetcdfFile copy) {
@@ -659,7 +666,11 @@ private boolean checkEach(String what, Object want1, String name1, List list1, S
   private boolean checkAtt(String what, Attribute want, String name1, AttributeContainer list1, String name2,
       AttributeContainer list2, ObjFilter objFilter) {
     boolean ok = true;
-    Attribute found = list2.findAttributeIgnoreCase(want.getShortName());
+    Attribute found;
+    if (this.ignoreattrcase)
+      found = list2.findAttributeIgnoreCase(want.getShortName());
+    else
+      found = list2.findAttribute(want.getShortName());
     if (found == null) {
       f.format("  ** %s: %s (%s) not in %s %n", what, want, name1, name2);
       ok = false;

cdm/core/src/test/java/ucar/nc2/TestSpecialAttributes.java

@@ -21,7 +21,7 @@ public class TestSpecialAttributes {
   @Test
   public void testReadAll() throws IOException {
     NetcdfFile ncfile = TestDir.openFileLocal("testSpecialAttributes.nc4");
-    // Iterate over all top-level attributes and see if it is special
+    // Iterate over all top-level visible attributes and see if it is special
     for (Attribute a : ncfile.getRootGroup().getAttributes()) {
       Assert.assertFalse("Attribute iteration found special attribute: " + a.getShortName(), Attribute.isspecial(a));
     }