Review comments

google · Mar 26, 2024 · 0695519 · 0695519
1 parent ba495b9
commit 0695519
Show file tree

Hide file tree

Showing 65 changed files with 8,569 additions and 398 deletions.
diff --git a/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java b/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/AvroConverter.java
@@ -7,7 +7,8 @@
 import com.cerner.bunsen.definitions.HapiConverter;
 import com.cerner.bunsen.definitions.HapiConverter.HapiObjectConverter;
 import com.cerner.bunsen.definitions.StructureDefinitions;
-import com.cerner.bunsen.exception.HapiMergeException;
+import com.cerner.bunsen.exception.ProfileException;
+import com.google.common.base.Preconditions;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
@@ -19,6 +20,7 @@
 import org.apache.avro.Schema;
 import org.apache.avro.Schema.Field;
 import org.apache.avro.generic.IndexedRecord;
+import org.apache.commons.collections.CollectionUtils;
 import org.hl7.fhir.instance.model.api.IBaseResource;
 
 /** Converter to change HAPI objects into Avro structures and vice versa. */
@@ -149,7 +151,7 @@ public static AvroConverter forResource(FhirContext context, String resourceType
    * @return the merged Avro converter
    */
   public static AvroConverter forResources(FhirContext context, List<String> resourceTypeUrls)
-      throws HapiMergeException {
+      throws ProfileException {
     List<AvroConverter> avroConverters = new ArrayList<>();
     for (String resourceTypeUrl : resourceTypeUrls) {
       AvroConverter avroConverter = forResource(context, resourceTypeUrl, Collections.emptyList());
@@ -227,11 +229,9 @@ public String getResourceType() {
    * of all the fields in the list of avroConverters
    */
   private static AvroConverter mergeAvroConverters(
-      List<AvroConverter> avroConverters, FhirContext context) throws HapiMergeException {
-    if (avroConverters == null || avroConverters.isEmpty()) {
-      throw new IllegalArgumentException("AvroConverter list cannot be empty for merging");
-    }
-
+      List<AvroConverter> avroConverters, FhirContext context) throws ProfileException {
+    Preconditions.checkArgument(
+        !CollectionUtils.isEmpty(avroConverters), "AvroConverter list cannot be empty for merging");
     Iterator<AvroConverter> iterator = avroConverters.iterator();
     AvroConverter mergedConverter = iterator.next();
     while (iterator.hasNext()) {
@@ -241,7 +241,7 @@ private static AvroConverter mergeAvroConverters(
   }
 
   private static AvroConverter mergeAvroConverters(
-      AvroConverter left, AvroConverter right, FhirContext context) throws HapiMergeException {
+      AvroConverter left, AvroConverter right, FhirContext context) throws ProfileException {
     HapiConverter<Schema> mergedConverter =
         left.hapiToAvroConverter.merge(right.hapiToAvroConverter);
     RuntimeResourceDefinition[] resources = new RuntimeResourceDefinition[1];

diff --git a/.../bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java b/.../bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/DefinitionToAvroVisitor.java
@@ -13,12 +13,13 @@
 import com.cerner.bunsen.definitions.HapiConverter.HapiFieldSetter;
 import com.cerner.bunsen.definitions.HapiConverter.HapiObjectConverter;
 import com.cerner.bunsen.definitions.HapiConverter.MultiValueConverter;
+import com.cerner.bunsen.definitions.HapiConverterUtil;
 import com.cerner.bunsen.definitions.IdConverter;
 import com.cerner.bunsen.definitions.LeafExtensionConverter;
 import com.cerner.bunsen.definitions.PrimitiveConverter;
 import com.cerner.bunsen.definitions.StringConverter;
 import com.cerner.bunsen.definitions.StructureField;
-import com.cerner.bunsen.exception.HapiMergeException;
+import com.cerner.bunsen.exception.ProfileException;
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
@@ -72,19 +73,9 @@ public Schema getDataType() {
         }
 
         @Override
-        public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-          if (other != null
-              && other instanceof PrimitiveConverter
-              && "Boolean".equals(other.getElementType())) {
-            return this;
-          }
-          throw new HapiMergeException(
-              String.format(
-                  "Cannot merge Boolean FHIR Type PrimitiveConverter with %s ",
-                  other != null
-                      ? String.format(
-                          "%s FHIR Type %s", other.getElementType(), other.getClass().getName())
-                      : null));
+        public HapiConverter merge(HapiConverter other) throws ProfileException {
+          HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
+          return this;
         }
       };
 
@@ -99,19 +90,9 @@ public Schema getDataType() {
         }
 
         @Override
-        public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-          if (other != null
-              && other instanceof PrimitiveConverter
-              && "Integer".equals(other.getElementType())) {
-            return this;
-          }
-          throw new HapiMergeException(
-              String.format(
-                  "Cannot merge Integer FHIR Type PrimitiveConverter with %s ",
-                  other != null
-                      ? String.format(
-                          "%s FHIR Type %s", other.getElementType(), other.getClass().getName())
-                      : null));
+        public HapiConverter merge(HapiConverter other) throws ProfileException {
+          HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
+          return this;
         }
       };
 
@@ -153,19 +134,9 @@ public Schema getDataType() {
         }
 
         @Override
-        public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-          if (other != null
-              && other instanceof PrimitiveConverter
-              && "Double".equals(other.getElementType())) {
-            return this;
-          }
-          throw new HapiMergeException(
-              String.format(
-                  "Cannot merge Double FHIR Type PrimitiveConverter with %s ",
-                  other != null
-                      ? String.format(
-                          "%s FHIR Type %s", other.getElementType(), other.getClass().getName())
-                      : null));
+        public HapiConverter merge(HapiConverter other) throws ProfileException {
+          HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
+          return this;
         }
       };
 
@@ -254,20 +225,14 @@ protected boolean isMultiValued(Schema schema) {
     }
 
     @Override
-    public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-      if (other == null || !(other instanceof CompositeToAvroConverter)) {
-        throw new HapiMergeException(
-            String.format(
-                "Cannot merge CompositeToAvroConverter with %s",
-                other != null ? other.getClass().getName() : null));
-      }
-
+    public HapiConverter merge(HapiConverter other) throws ProfileException {
+      HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
       CompositeToAvroConverter otherConverter = (CompositeToAvroConverter) other;
       // For extensions
       if (!(Strings.isNullOrEmpty(this.extensionUrl())
           && Strings.isNullOrEmpty(otherConverter.extensionUrl()))) {
         if (!Objects.equals(this.extensionUrl(), otherConverter.extensionUrl())) {
-          throw new HapiMergeException(
+          throw new ProfileException(
               String.format(
                   "The extension URL must be the same to merge. currentExtensionUrl=%s,"
                       + " otherExtensionUrl=%s",
@@ -283,11 +248,9 @@ public HapiConverter merge(HapiConverter other) throws HapiMergeException {
 
       List<StructureField<HapiConverter<Schema>>> mergedList = new ArrayList<>();
       Map<String, StructureField> currentElementsMap =
-          currentElements.stream()
-              .collect(Collectors.toMap(structureField -> structureField.fieldName(), s -> s));
+          currentElements.stream().collect(Collectors.toMap(s -> s.fieldName(), s -> s));
       Map<String, StructureField> otherElementsMap =
-          rightElements.stream()
-              .collect(Collectors.toMap(structureField -> structureField.fieldName(), s -> s));
+          rightElements.stream().collect(Collectors.toMap(s -> s.fieldName(), s -> s));
 
       for (StructureField currentField : currentElements) {
         if (!otherElementsMap.containsKey(currentField.fieldName())) {
@@ -364,14 +327,8 @@ protected Object createComposite(Object[] children) {
     }
 
     @Override
-    public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-      if (other == null || !(other instanceof HapiChoiceToAvroConverter)) {
-        throw new HapiMergeException(
-            String.format(
-                "Cannot merge HapiChoiceToAvroConverter with %s",
-                other != null ? other.getClass().getName() : null));
-      }
-
+    public HapiConverter merge(HapiConverter other) throws ProfileException {
+      HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
       HapiChoiceToAvroConverter otherConverter = (HapiChoiceToAvroConverter) other;
       Map<String, HapiConverter<Schema>> currentChoiceTypes = this.getElements();
       Map<String, HapiConverter<Schema>> otherChoiceTypes = otherConverter.getElements();
@@ -490,8 +447,8 @@ protected Object createContained(Object[] contained) {
     }
 
     @Override
-    public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-      throw new HapiMergeException("Merging of HapiContainedToAvroConverter is not supported");
+    public HapiConverter merge(HapiConverter other) throws ProfileException {
+      throw new ProfileException("Merging of HapiContainedToAvroConverter is not supported");
     }
   }
 
@@ -567,14 +524,8 @@ public HapiFieldSetter toHapiConverter(BaseRuntimeElementDefinition... elementDe
     }
 
     @Override
-    public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-      if (other == null || !(other instanceof MultiValuedToAvroConverter)) {
-        throw new HapiMergeException(
-            String.format(
-                "Cannot merge MultiValuedToAvroConverter with %s",
-                other != null ? other.getClass().getName() : null));
-      }
-
+    public HapiConverter merge(HapiConverter other) throws ProfileException {
+      HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
       HapiConverter mergedElementConverter =
           this.elementConverter.merge(((MultiValuedToAvroConverter) other).getElementConverter());
       return new MultiValuedToAvroConverter(mergedElementConverter);
@@ -737,18 +688,12 @@ public HapiFieldSetter toHapiConverter(BaseRuntimeElementDefinition... elementDe
     }
 
     @Override
-    public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-      if (other == null || !(other instanceof RelativeValueConverter)) {
-        throw new HapiMergeException(
-            String.format(
-                "Cannot merge RelativeValueConverter with %s",
-                other != null ? other.getClass().getName() : null));
-      }
-
+    public HapiConverter merge(HapiConverter other) throws ProfileException {
+      HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
       String leftPrefix = Strings.nullToEmpty(this.prefix);
       String rightPrefix = Strings.nullToEmpty(((RelativeValueConverter) other).prefix);
       if (!Objects.equals(leftPrefix, rightPrefix)) {
-        throw new HapiMergeException(
+        throw new ProfileException(
             String.format(
                 "RelativeValueConverter prefixes should be equal for merging, leftPrefix=%s,"
                     + " rightPrefix=%s",

diff --git a/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/NoOpConverter.java b/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/converters/NoOpConverter.java
@@ -3,7 +3,8 @@
 import ca.uhn.fhir.context.BaseRuntimeChildDefinition;
 import ca.uhn.fhir.context.BaseRuntimeElementDefinition;
 import com.cerner.bunsen.definitions.HapiConverter;
-import com.cerner.bunsen.exception.HapiMergeException;
+import com.cerner.bunsen.definitions.HapiConverterUtil;
+import com.cerner.bunsen.exception.ProfileException;
 import org.apache.avro.Schema;
 import org.apache.avro.Schema.Type;
 import org.hl7.fhir.instance.model.api.IBase;
@@ -38,14 +39,9 @@ public HapiFieldSetter toHapiConverter(BaseRuntimeElementDefinition... elementDe
   }
 
   @Override
-  public HapiConverter merge(HapiConverter other) throws HapiMergeException {
-    if (other != null && other instanceof NoOpConverter) {
-      return this;
-    }
-    throw new HapiMergeException(
-        String.format(
-            "Cannot merge NoOpConverter with %s",
-            other != null ? other.getClass().getName() : null));
+  public HapiConverter merge(HapiConverter other) throws ProfileException {
+    HapiConverterUtil.validateIfConvertersCanBeMerged(this, other);
+    return this;
   }
 
   public static final NoOpConverter INSTANCE = new NoOpConverter();

diff --git a/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/tools/GenerateSchemas.java b/bunsen/bunsen-avro/src/main/java/com/cerner/bunsen/avro/tools/GenerateSchemas.java
@@ -4,7 +4,7 @@
 import ca.uhn.fhir.context.FhirVersionEnum;
 import com.cerner.bunsen.ProfileMapperFhirContexts;
 import com.cerner.bunsen.avro.AvroConverter;
-import com.cerner.bunsen.exception.ProfileMapperException;
+import com.cerner.bunsen.exception.ProfileException;
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -27,7 +27,7 @@ public class GenerateSchemas {
    * @param args the output file followed by a list of resource type urls
    * @return the OS status code
    */
-  public static int main(String[] args) throws ProfileMapperException {
+  public static int main(String[] args) throws ProfileException {
 
     if (args.length < 2) {
       System.out.println("Usage: GenerateSchemas <output file> resourceTypeUrls...");