Regularize the scoping of types

re: Github issue Unidata#1956

The function NC_compare_nc_types in libdispatch/dcopy.c uses an
incorrect algorithm to search for types. The core of this is the
function NC_rec_find_nc_type in libdispatch/dcopy.c. Currently
it searchs the current group and its subtree.

Additionally, the function NC4_inq_typeid in libsrc4/nc4internal.c
has been extended to handle fully qualified names. It was originally
designed to do this, but for some reason never completed.

The NC_rec_find_nc_type algorithm has been altered to match the
algorithm used by NC4_inq_typeid. It operates as follows.

Given a file F, group G and a type T. It searches file F2, group
G2, for another type T2 that is equivalent to T.

The search order is as follows.
1. Search G2 for a type T2 equivalent to T.
2. Search upwards in the ancestor groups of G2 for a type T2 equivalent to T.
3. Search the complete group tree of F2 in pre-order, breadth-first order to locate T2 equivalent to T.

Also add a test case to validate algorithm: ncdump/test_scope.sh.

Note, this change may cause compatibility problems, though it is
unlikely because two different equivalent type declarations in
one dataset is unlikely.

.github/workflows/run_tests.yml

@@ -4,7 +4,7 @@
 
 name: Run netCDF Tests
 
-on: [pull_request]
+on: [pull_request,push]
 
 jobs:
 

include/ncconfigure.h

@@ -16,6 +16,9 @@
 #ifdef HAVE_STDIO_H
 #include <stdio.h>
 #endif
+#ifdef HAVE_STDINT_H
+#include <stdint.h>
+#endif
 
 /*
 This is included in bottom

libdispatch/dcopy.c

@@ -10,8 +10,13 @@
 #include "config.h"
 #include "ncdispatch.h"
 #include "nc_logging.h"
+#include "nclist.h"
 
 #ifdef USE_NETCDF4
+
+static int searchgroup(int ncid1, int tid1, int grp, int* tid2);
+static int searchgrouptree(int ncid1, int tid1, int grp, int* tid2);
+
 /**
  * @internal Compare two netcdf types for equality. Must have the
  * ncids as well, to find user-defined types.
@@ -27,8 +32,7 @@
  * @author Ed Hartnett
 */
 static int
-NC_compare_nc_types(int ncid1, int typeid1, int ncid2, int typeid2,
-		    int *equalp)
+NC_compare_nc_types(int ncid1, int typeid1, int ncid2, int typeid2, int *equalp)
 {
    int ret = NC_NOERR;
 
@@ -152,77 +156,51 @@ NC_compare_nc_types(int ncid1, int typeid1, int ncid2, int typeid2,
 }
 
 /**
- * @internal Recursively hunt for a netCDF type id. (Code from
- * nc4internal.c); Return matching typeid or 0 if not found.
+ * @internal Recursively hunt for a netCDF type id, tid2, that is "equal" to tid1.
+ * Question is: what search order do we use? Ncgen uses root group tree in pre-order.
+ * But NC4_inq_typeid uses these rules:
+ * 1. ncid2
+ * 2. parents of ncid2 (up the tree to root)
+ * 3. root group tree in pre-order.
+ * We will leave ncgen for another day and use the nc_inq_typeid rule.
+ *
+ * Return matching typeid or 0 if not found.
  *
  * @param ncid1 File ID.
  * @param tid1 Type ID.
  * @param ncid2 File ID.
  * @param tid2 Pointer that gets type ID of equal type.
  *
  * @return ::NC_NOERR No error.
- * @author Ed Hartnett
+ * @author Ed Hartnett, Dennis Heimbigner
 */
 static int
 NC_rec_find_nc_type(int ncid1, nc_type tid1, int ncid2, nc_type* tid2)
 {
-   int i,ret = NC_NOERR;
-   int nids;
-   int* ids = NULL;
-
-   /* Get all types in grp ncid2 */
-   if(tid2)
-      *tid2 = 0;
-   if ((ret = nc_inq_typeids(ncid2, &nids, NULL)))
-      return ret;
-   if (nids)
-   {
-      if (!(ids = (int *)malloc((size_t)nids * sizeof(int))))
-	 return NC_ENOMEM;
-      if ((ret = nc_inq_typeids(ncid2, &nids, ids)))
-	 return ret;
-      for(i = 0; i < nids; i++)
-      {
-	 int equal = 0;
-	 if ((ret = NC_compare_nc_types(ncid1, tid1, ncid2, ids[i], &equal)))
-	    return ret;
-	 if(equal)
-	 {
-	    if(tid2)
-	       *tid2 = ids[i];
-	    free(ids);
-	    return NC_NOERR;
-	 }
-      }
-      free(ids);
+    int ret = NC_NOERR;
+    int parent;
+
+    if((ret = searchgroup(ncid1,tid1,ncid2,tid2)))
+        goto done;
+    if(*tid2 != 0)
+        goto done; /* found */
+
+   /* Look in the parents of ncid2 upto the root */
+   switch (ret = nc_inq_grp_parent(ncid2,&parent)) {
+   case NC_NOERR:
+	/* Recurse up using parent grp */
+        ret = NC_rec_find_nc_type(ncid1, tid1, parent, tid2);
+	break;
+   case NC_ENOGRP:
+	/* do the breadth-first pre-order search of the whole tree */
+	/* ncid2 should be root group */
+	ret = searchgrouptree(ncid1,tid1,ncid2,tid2);
+	break;
+   default: break;
    }
 
-   /* recurse */
-   if ((ret = nc_inq_grps(ncid1, &nids, NULL)))
-      return ret;
-   if (nids)
-   {
-      if (!(ids = (int *)malloc((size_t)nids * sizeof(int))))
-	 return NC_ENOMEM;
-      if ((ret = nc_inq_grps(ncid1, &nids, ids)))
-      {
-	 free(ids);
-	 return ret;
-      }
-      for (i = 0; i < nids; i++)
-      {
-	 ret = NC_rec_find_nc_type(ncid1, tid1, ids[i], tid2);
-	 if (ret && ret != NC_EBADTYPE)
-	    break;
-	 if (tid2 && *tid2 != 0) /* found */
-	 {
-	    free(ids);
-	    return NC_NOERR;
-	 }
-      }
-      free(ids);
-   }
-   return NC_EBADTYPE; /* not found */
+done:
+    return ret;
 }
 
 /**
@@ -711,3 +689,92 @@ nc_copy_att(int ncid_in, int varid_in, const char *name,
 
    return NC_NOERR;
 }
+
+#ifdef USE_NETCDF4
+
+/* Helper function for NC_rec_find_nc_type();
+   search a specified group for matching type.
+*/
+static int
+searchgroup(int ncid1, int tid1, int grp, int* tid2)
+{
+    int i,ret = NC_NOERR;
+    int nids;
+    int* ids = NULL;
+
+    /* Get all types in grp */
+    if(tid2)
+	*tid2 = 0;
+    if ((ret = nc_inq_typeids(grp, &nids, NULL)))
+	goto done;
+    if (nids)
+    {
+	if (!(ids = (int *)malloc((size_t)nids * sizeof(int))))
+	    {ret = NC_ENOMEM; goto done;}
+	if ((ret = nc_inq_typeids(grp, &nids, ids)))
+	    goto done;
+	for(i = 0; i < nids; i++)
+        {
+	    int equal = 0;
+	    if ((ret = NC_compare_nc_types(ncid1, tid1, grp, ids[i], &equal)))
+	        goto done;
+	    if(equal)
+	    {
+		if(tid2)
+		    *tid2 = ids[i];
+		goto done;
+	    }
+	}
+    }
+
+done:
+    nullfree(ids);
+    return ret;
+}
+
+/* Helper function for NC_rec_find_nc_type();
+   search a tree of groups for a matching type
+   using a breadth first queue
+*/
+static int
+searchgrouptree(int ncid1, int tid1, int grp, int* tid2)
+{
+    int i,ret = NC_NOERR;
+    int nids;
+    int* ids = NULL;
+    NClist* queue = nclistnew();
+    int gid;
+    uintptr_t id;
+
+    id = grp;
+    nclistpush(queue,(void*)id); /* prime the queue */
+    while(nclistlength(queue) > 0) {
+        id = (uintptr_t)nclistremove(queue,0);
+	gid  = (int)id;
+        if((ret = searchgroup(ncid1,tid1,gid,tid2)))
+            goto done;
+        if(*tid2 != 0)
+            goto done; /*we found it*/
+	/* Get subgroups of gid and push onto front of the queue (for breadth first) */
+        if((ret = nc_inq_grps(gid,&nids,NULL)))
+            goto done;
+        if (!(ids = (int *)malloc((size_t)nids * sizeof(int))))
+	    {ret = NC_ENOMEM; goto done;}
+        if ((ret = nc_inq_grps(gid, &nids, ids)))
+            goto done;
+	/* push onto the end of the queue */
+        for(i=0;i<nids;i++) {
+	    id = ids[i];
+	    nclistpush(queue,(void*)id);
+	}
+    }
+    /* Not found */
+    ret = NC_EBADTYPE;
+
+done:
+    nclistfree(queue);
+    nullfree(ids);
+    return ret;
+}
+
+#endif

libsrc4/nc4type.c

@@ -554,36 +554,56 @@ NC4_inq_typeid(int ncid, const char *name, nc_type *typeidp)
     NC_GRP_INFO_T *grptwo;
     NC_FILE_INFO_T *h5;
     NC_TYPE_INFO_T *type = NULL;
-    char *norm_name;
-    int i, retval;
+    char *norm_name = NULL;
+    int i, retval = NC_NOERR;
 
     /* Handle atomic types. */
     for (i = 0; i < NUM_ATOMIC_TYPES; i++)
         if (!strcmp(name, nc4_atomic_name[i]))
         {
             if (typeidp)
                 *typeidp = i;
-            return NC_NOERR;
+            goto done;
         }
 
     /* Find info for this file and group, and set pointer to each. */
     if ((retval = nc4_find_grp_h5(ncid, &grp, &h5)))
-        return retval;
+        goto done;
     assert(h5 && grp);
 
     /* If the first char is a /, this is a fully-qualified
      * name. Otherwise, this had better be a local name (i.e. no / in
      * the middle). */
     if (name[0] != '/' && strstr(name, "/"))
-        return NC_EINVAL;
+        {retval = NC_EINVAL; goto done;}
 
     /* Normalize name. */
     if (!(norm_name = (char*)malloc(strlen(name) + 1)))
-        return NC_ENOMEM;
-    if ((retval = nc4_normalize_name(name, norm_name))) {
-        free(norm_name);
-        return retval;
+        {retval = NC_ENOMEM; goto done;}
+    if ((retval = nc4_normalize_name(name, norm_name)))
+	goto done;
+
+    /* If this is a fqn, then walk the sequence of parent groups to the last group
+       and see if that group has a type of the right name */
+    if(name[0] == '/') { /* FQN */
+	int rootncid = (grp->nc4_info->root_grp->hdr.id | grp->nc4_info->controller->ext_ncid);
+	int parent = 0;
+	char* lastname = strrchr(norm_name,'/'); /* break off the last segment: the type name */
+	if(lastname == norm_name)
+	    {retval = NC_EINVAL; goto done;}
+	*lastname++ = '\0'; /* break off the lastsegment */
+	if((retval = NC4_inq_grp_full_ncid(rootncid,norm_name,&parent))) 
+	    goto done;
+	/* Get parent info */
+	if((retval=nc4_find_nc4_grp(parent,&grp)))
+	    goto done;
+	/* See if type exists in this group */
+        type = (NC_TYPE_INFO_T*)ncindexlookup(grp->type,lastname);
+	if(type == NULL) 	
+	    {retval = NC_EBADTYPE; goto done;}
+	goto done;
     }
+
     /* Is the type in this group? If not, search parents. */
     for (grptwo = grp; grptwo; grptwo = grptwo->parent) {
         type = (NC_TYPE_INFO_T*)ncindexlookup(grptwo->type,norm_name);
@@ -602,13 +622,13 @@ NC4_inq_typeid(int ncid, const char *name, nc_type *typeidp)
             if (typeidp)
                 *typeidp = type->hdr.id;
 
-    free(norm_name);
-
     /* OK, I give up already! */
     if (!type)
-        return NC_EBADTYPE;
+        {retval = NC_EBADTYPE; goto done;}
 
-    return NC_NOERR;
+done:
+    nullfree(norm_name);
+    return retval;
 }
 
 /**

ncdump/CMakeLists.txt

@@ -13,6 +13,7 @@ SET(ncdump_FILES ncdump.c vardata.c dumplib.c indent.c nctime0.c utils.c nciter.
 SET(nccopy_FILES nccopy.c nciter.c chunkspec.c utils.c dimmap.c list.c)
 SET(ocprint_FILES ocprint.c)
 SET(ncvalidator_FILES ncvalidator.c)
+SET(printfqn_FILES printfqn.c)
 
 IF(USE_X_GETOPT)
   SET(ncdump_FILES ${ncdump_FILES} XGetopt.c)
@@ -27,6 +28,7 @@ ADD_EXECUTABLE(ncvalidator ${ncvalidator_FILES})
 
 IF(USE_HDF5)
   ADD_EXECUTABLE(nc4print nc4print.c nc4printer.c)
+  ADD_EXECUTABLE(printfqn  ${printfqn_FILES})
 ENDIF(USE_HDF5)
 
 IF(ENABLE_DAP)
@@ -39,6 +41,7 @@ TARGET_LINK_LIBRARIES(ncvalidator netcdf ${ALL_TLL_LIBS})
 
 IF(USE_HDF5)
   TARGET_LINK_LIBRARIES(nc4print netcdf ${ALL_TLL_LIBS})
+  TARGET_LINK_LIBRARIES(printfqn netcdf ${ALL_TLL_LIBS})
 ENDIF(USE_HDF5)
 
 IF(ENABLE_DAP)
@@ -81,6 +84,15 @@ IF(MSVC)
       ${CMAKE_CURRENT_BINARY_DIR})
     SET_TARGET_PROPERTIES(ncvalidator PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE
       ${CMAKE_CURRENT_BINARY_DIR})
+
+    SET_TARGET_PROPERTIES(printfqn PROPERTIES RUNTIME_OUTPUT_DIRECTORY
+      ${CMAKE_CURRENT_BINARY_DIR})
+    SET_TARGET_PROPERTIES(printfqn PROPERTIES RUNTIME_OUTPUT_DIRECTORY_DEBUG
+      ${CMAKE_CURRENT_BINARY_DIR})
+    SET_TARGET_PROPERTIES(printfqn PROPERTIES RUNTIME_OUTPUT_DIRECTORY_RELEASE
+      ${CMAKE_CURRENT_BINARY_DIR})
+
+
   ENDIF(USE_HDF5)
 
   IF(ENABLE_DAP)
@@ -295,6 +307,10 @@ ENDIF(MSVC)
   add_sh_test(ncdump test_keywords)
   ENDIF()
 
+  IF(USE_CDF5)
+  add_sh_test(ncdump test_scope)
+  ENDIF()
+
 ENDIF()
 
 ENDIF()