Skip to content

Commit

Permalink
Fix bug in partial CF Sub-convention name matches
Browse files Browse the repository at this point in the history 
Make sure CF Sub-conventions work even if the full conventions name
attribute is not a match. Add the partial match case to the existing
test.
  • Loading branch information
@lesserwhirls
lesserwhirls authored and haileyajohnson committed Jun 27, 2022
1 parent e69c6e2 commit f391525
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 21 deletions.
9 changes: 4 additions & 5 deletions cdm/core/src/main/java/ucar/nc2/internal/dataset/CoordSystemFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -230,13 +230,12 @@ public static Optional<CoordSystemBuilder> factory(NetcdfDataset.Builder ds, Can
}

// if the match is on the main CF convention, we need to look at possible sub-conventions or incubating conventions
if (coordSysFactory != null && ds.orgFile != null && coordSysFactory.getClass().isInstance(CF1Convention.class)) {
if (coordSysFactory != null && ds.orgFile != null && coordSysFactory instanceof CF1Convention.Factory) {
List<String> convs = breakupConventionNames(convName);
if (convs.size() > 1) {
CoordSystemBuilderFactory potentialCoordSysFactory = findCfSubConvention(ds.orgFile);
// only use the potentialCoordSysFactory if it is a CF sub-convention
if (potentialCoordSysFactory != null
&& potentialCoordSysFactory.getClass().isInstance(CFSubConventionProvider.class)) {
if (potentialCoordSysFactory != null && potentialCoordSysFactory instanceof CFSubConventionProvider) {
coordSysFactory = potentialCoordSysFactory;
}
}
Expand Down Expand Up @@ -294,14 +293,14 @@ private static CoordSystemBuilderFactory findCfSubConvention(NetcdfFile orgFile)
// Look for Convention using isMine()
for (Convention conv : conventionList) {
CoordSystemBuilderFactory candidate = conv.factory;
if (candidate.getClass().isInstance(CFSubConventionProvider.class) && candidate.isMine(orgFile)) {
if (candidate instanceof CFSubConventionProvider && candidate.isMine(orgFile)) {
return candidate;
}
}

// Use service loader mechanism isMine()
for (CoordSystemBuilderFactory csb : ServiceLoader.load(CoordSystemBuilderFactory.class)) {
if (csb.getClass().isInstance(CFSubConventionProvider.class) && csb.isMine(orgFile)) {
if (csb instanceof CFSubConventionProvider && csb.isMine(orgFile)) {
return csb;
}
}
Expand Down
53 changes: 39 additions & 14 deletions cdm/core/src/test/java/ucar/nc2/dataset/TestCfSubConventionProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,17 +19,21 @@
import static com.google.common.truth.Truth.assertThat;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Optional;

public class TestCfSubConventionProvider {

private static final NetcdfDataset.Builder<?> CF_NCDB = NetcdfDataset.builder();
private static final NetcdfDataset.Builder<?> YOLO_NCDB = NetcdfDataset.builder();
private static final NetcdfDataset.Builder<?> YOLO_EXACT_NCDB = NetcdfDataset.builder();
private static final NetcdfDataset.Builder<?> YOLO_PARTIAL_NCDB = NetcdfDataset.builder();

@BeforeClass
public static void makeNcdBuilders() throws IOException {
NetcdfFile.Builder<?> cfNcfb = NetcdfFile.builder();
NetcdfFile.Builder<?> yoloNcfb = NetcdfFile.builder();
NetcdfFile.Builder<?> yoloExactNcfb = NetcdfFile.builder();
NetcdfFile.Builder<?> yoloPartialNcfb = NetcdfFile.builder();

Group.Builder cfRoot = Group.builder().setName("");
cfNcfb.setRootGroup(cfRoot);
Expand All @@ -42,14 +46,30 @@ public static void makeNcdBuilders() throws IOException {
throw new IOException("Error building NetcdfFile object to mock a CF netCDF file for testing.", ioe);
}

Group.Builder yoloRoot = Group.builder().setName("");
yoloNcfb.setRootGroup(yoloRoot);
Attribute yoloConvAttr =
Group.Builder yoloExactRoot = Group.builder().setName("");
yoloExactNcfb.setRootGroup(yoloExactRoot);
// exactly match the convention name
Attribute yoloExactConvAttr =
Attribute.builder(CDM.CONVENTIONS).setStringValue(CfSubConvForTest.CONVENTION_NAME).build();
yoloRoot.addAttribute(yoloConvAttr);
try (NetcdfFile yoloNcf = yoloNcfb.build()) {
YOLO_NCDB.copyFrom(yoloNcf);
YOLO_NCDB.setOrgFile(yoloNcf);
yoloExactRoot.addAttribute(yoloExactConvAttr);
try (NetcdfFile yoloExactNcf = yoloExactNcfb.build()) {
YOLO_EXACT_NCDB.copyFrom(yoloExactNcf);
YOLO_EXACT_NCDB.setOrgFile(yoloExactNcf);
} catch (IOException ioe) {
throw new IOException("Error building NetcdfFile object to mock a CF/YOLO netCDF file for testing.", ioe);
}

Group.Builder yoloPartialRoot = Group.builder().setName("");
yoloPartialNcfb.setRootGroup(yoloPartialRoot);
// change up the convention name used so that it is not an exact match with the test convention name
Attribute yoloPartialConvAttr = Attribute.builder(CDM.CONVENTIONS)
.setStringValue(
CfSubConvForTest.CONVENTION_NAME.replaceFirst(CfSubConvForTest.CONVENTAION_NAME_STARTS_WITH, "CF-1.200"))
.build();
yoloPartialRoot.addAttribute(yoloPartialConvAttr);
try (NetcdfFile yoloPartialNcf = yoloPartialNcfb.build()) {
YOLO_PARTIAL_NCDB.copyFrom(yoloPartialNcf);
YOLO_PARTIAL_NCDB.setOrgFile(yoloPartialNcf);
} catch (IOException ioe) {
throw new IOException("Error building NetcdfFile object to mock a CF/YOLO netCDF file for testing.", ioe);
}
Expand All @@ -62,10 +82,15 @@ public void testCfSubLoadOrder() throws IOException {
CoordSystemBuilder cfFac = cfFacOpt.get();
assertThat(cfFac).isInstanceOf(CF1Convention.class);

Optional<CoordSystemBuilder> yoloFacOpt = CoordSystemFactory.factory(YOLO_NCDB, null);
assertThat(yoloFacOpt.isPresent());
CoordSystemBuilder yoloFac = yoloFacOpt.get();
assertThat(yoloFac).isInstanceOf(CfSubConvForTest.class);
assertThat(yoloFac.getConventionUsed()).isEqualTo(CfSubConvForTest.CONVENTION_NAME);
// both datasets (exact and partial matches of the sub-convention name) should result in the
// sub-convention being used.
List<NetcdfDataset.Builder<?>> subConvBuilders = Arrays.asList(YOLO_EXACT_NCDB, YOLO_PARTIAL_NCDB);
for (NetcdfDataset.Builder<?> subConvBuilder : subConvBuilders) {
Optional<CoordSystemBuilder> yoloFacOpt = CoordSystemFactory.factory(subConvBuilder, null);
assertThat(yoloFacOpt.isPresent());
CoordSystemBuilder yoloFac = yoloFacOpt.get();
assertThat(yoloFac).isInstanceOf(CfSubConvForTest.class);
assertThat(yoloFac.getConventionUsed()).isEqualTo(CfSubConvForTest.CONVENTION_NAME);
}
}
}
17 changes: 15 additions & 2 deletions cdm/core/src/test/java/ucar/nc2/dataset/conv/CfSubConvForTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,20 @@
import ucar.nc2.internal.dataset.CoordSystemBuilder;
import ucar.nc2.internal.dataset.spi.CFSubConventionProvider;

import java.util.List;

import static ucar.nc2.internal.dataset.CoordSystemFactory.breakupConventionNames;

/**
* An implementation of CoordSystemBuilder for testing CFSubConvention hooks.
*
* @see ucar.nc2.dataset.TestCfSubConventionProvider
*/
public class CfSubConvForTest extends CoordSystemBuilder {
public static String CONVENTION_NAME = "CF-1.200/YOLO";
private static String SUBCONVENTAION_NAME = "YOLO";
// public for testing
public static String CONVENTAION_NAME_STARTS_WITH = "CF-1.X";
public static String CONVENTION_NAME = CONVENTAION_NAME_STARTS_WITH + "/" + SUBCONVENTAION_NAME;

private CfSubConvForTest(NetcdfDataset.Builder<?> datasetBuilder) {
super(datasetBuilder);
Expand All @@ -33,7 +40,13 @@ public boolean isMine(NetcdfFile ncfile) {
if (conventionAttr != null) {
String conventionValue = conventionAttr.getStringValue();
if (conventionValue != null) {
mine = conventionValue.equals(CONVENTION_NAME);
List<String> names = breakupConventionNames(conventionValue);
for (String name : names) {
if (name.equalsIgnoreCase(SUBCONVENTAION_NAME)) {
mine = true;
break;
}
}
}
}
return mine;
Expand Down

0 comments on commit f391525

Please sign in to comment.