Skip to content

Commit

Permalink
Merge pull request #3662 from IBM/issue-3661
Browse files Browse the repository at this point in the history 
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre authored Jun 9, 2022
2 parents cec279a + 6692b59 commit 4d74d9f
Show file tree
Hide file tree
Showing 5 changed files with 98 additions and 75 deletions.
18 changes: 8 additions & 10 deletions ...ance/fhir-ig-us-core/src/test/java/com/ibm/fhir/ig/us/core/test/v311/ConformanceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,11 @@ public void testUSCoreEthnicityExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-ethnicity|3.1.1"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 0);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

/**
Expand Down Expand Up @@ -527,12 +526,11 @@ public void testUSCoreRaceExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-race>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-race|3.1.1"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 1);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

}
18 changes: 8 additions & 10 deletions ...ance/fhir-ig-us-core/src/test/java/com/ibm/fhir/ig/us/core/test/v400/ConformanceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,11 @@ public void testUSCoreEthnicityExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-ethnicity|4.0.0"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 1);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

/**
Expand Down Expand Up @@ -519,12 +518,11 @@ public void testUSCoreRaceExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-race>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-race|4.0.0"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 1);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

@Test
Expand Down
18 changes: 8 additions & 10 deletions ...ance/fhir-ig-us-core/src/test/java/com/ibm/fhir/ig/us/core/test/v500/ConformanceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,12 +333,11 @@ public void testUSCoreEthnicityExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-ethnicity>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-ethnicity"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 1);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

/**
Expand Down Expand Up @@ -519,12 +518,11 @@ public void testUSCoreRaceExtension4() throws Exception {

issues.forEach(System.out::println);

// one for generated-ext-1: Extension must conform to definition 'http://hl7.org/fhir/us/core/StructureDefinition/us-core-race'
// and one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countErrors(issues), 4);
// one for generated-us-core-patient-Patient.extension<http://hl7.org/fhir/us/core/StructureDefinition/us-core-race>
// and one for the corresponding "not a valid member of ValueSet http://hl7.org/fhir/us/core/ValueSet/detailed-race"
Assert.assertEquals(countErrors(issues), 2);
Assert.assertEquals(countWarnings(issues), 1);
// one for each version of the extension due to the generated constraint "conformsTo('a'|1) or conformsTo('a'|2) or conformsTo('a'|3)..."
Assert.assertEquals(countInformation(issues), 3);
Assert.assertEquals(countInformation(issues), 1);
}

@Test
Expand Down
34 changes: 28 additions & 6 deletions fhir-profile/src/main/java/com/ibm/fhir/profile/ConstraintGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ public List<Constraint> generate() {
List<Constraint> constraints = new ArrayList<>();

String url = profile.getUrl().getValue();
boolean isProfile = !"Extension".equals(url);
String prefix = url.substring(url.lastIndexOf("/") + 1);

int index = 1;
Expand All @@ -82,13 +83,27 @@ public List<Constraint> generate() {
log.finest("Element definition -> constraint expression:");
for (Node child : tree.root.children) {
try {
String constraintId;
if (isProfile && hasExtensionConstraint(child.elementDefinition)) {
// We include both the path and the profile URL in the constraint id so that
// we can pull these out during validation and avoid redundant extension validation
Type type = getTypes(child.elementDefinition).get(0);
String profile = getProfilesWithoutVersion(type).get(0);

// https://www.rfc-editor.org/rfc/rfc3986.html#appendix-C explicitly mentions angle brackets
// as a good way to embed URIs in plain text.
// Other safe options include whitespace, curly braces, angle brackets, and/or backslash.
constraintId = "generated-" + prefix + "~" + child.elementDefinition.getPath().getValue() + "<" + profile + ">";
} else {
constraintId = "generated-" + prefix + "-" + index;
index++;
}
String expr = generate(child);
if (generated.contains(expr)) {
continue;
}
String description = "Constraint violation: " + expr;
constraints.add(constraint("generated-" + prefix + "-" + index, expr, description));
index++;
constraints.add(constraint(constraintId, expr, description));
generated.add(expr);
} catch (ConstraintGenerationException e) {
log.warning("Constraint was not generated due to the following error: " + e.getMessage() + " (elementDefinition: " + e.getElementDefinition().getId() + ", profile: " + url + ")");
Expand Down Expand Up @@ -492,17 +507,20 @@ private String generate(Node node) {

/**
* Guard calls to this method with calls to {@link #hasExtensionConstraint(ElementDefinition)}
* to ensure that the node's ElementDefinition has a single type with a single profile.
* to ensure that the node's ElementDefinition has a single type with a single profile value.
*/
private String generateExtensionConstraint(Node node) {
StringBuilder sb = new StringBuilder();

ElementDefinition elementDefinition = node.elementDefinition;

Type type = getTypes(elementDefinition).get(0);
String profile = getProfilesWithoutVersion(type).get(0);
String profile = type.getProfile().get(0).getValue();
String profileWithoutVersion = profile.contains("|") ? profile.substring(0, profile.lastIndexOf("|")) : profile;

sb.append("extension('").append(profile).append("')").append(cardinality(node, sb.toString()));
sb.append("extension('").append(profileWithoutVersion).append("')").append(cardinality(node, sb.toString()));
sb.append(" and ");
sb.append("extension('").append(profileWithoutVersion).append("')").append(".all(conformsTo('" + profile + "'))");

return sb.toString();
}
Expand Down Expand Up @@ -610,7 +628,7 @@ private String generatePatternValueConstraint(Node node, boolean discriminator)
*/
private String generateProfileConstraint(Node node) {
StringBuilder sb = new StringBuilder();

ElementDefinition elementDefinition = node.elementDefinition;

List<Type> types = getTypes(elementDefinition);
Expand Down Expand Up @@ -862,6 +880,10 @@ private boolean hasExtensionConstraint(ElementDefinition elementDefinition) {
return false;
}

if (profile.get(0).getValue() == null) {
return false;
}

return true;
}

Expand Down
85 changes: 46 additions & 39 deletions fhir-validation/src/main/java/com/ibm/fhir/validation/FHIRValidator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@
import java.util.concurrent.ConcurrentHashMap;
import java.util.logging.Level;
import java.util.logging.Logger;
import java.util.stream.Collectors;

import com.ibm.fhir.model.annotation.Constraint;
import com.ibm.fhir.model.annotation.Constraint.FHIRPathConstraintValidator;
Expand All @@ -64,7 +63,6 @@
import com.ibm.fhir.path.visitor.FHIRPathDefaultNodeVisitor;
import com.ibm.fhir.profile.ProfileSupport;
import com.ibm.fhir.registry.FHIRRegistry;
import com.ibm.fhir.registry.resource.FHIRRegistryResource;
import com.ibm.fhir.validation.exception.FHIRValidationException;

import net.jcip.annotations.NotThreadSafe;
Expand Down Expand Up @@ -332,64 +330,73 @@ private void validate(FHIRPathResourceNode resourceNode) {
constraints.addAll(ProfileSupport.getConstraints(profiles, resourceType));
}

// add instance-specific extension constraints
PathAwareCollectingVisitor<Extension> extCollector = new PathAwareCollectingVisitor<Extension>(Extension.class);
resourceNode.resource().accept(extCollector);
Map<String, Extension> pathToExtension = extCollector.getResult();

// for option A below: find all the versions of the extension in the registry
Map<String, Set<String>> profileVersions = collectProfileVersions(pathToExtension.values());
// path (with array indices) -> Extension instance (which never have a url version suffix)
Map<String, Extension> pathToExtension = gatherInstanceExtensions(resourceNode);
// path (with no array indices) -> Extension URLs (with no version suffix)
Map<String, Set<String>> alreadyGeneratedExtensionConstraints = gatherExtensionConstraints(constraints);

// for each extension in the resource instance
for (Entry<String, Extension> entry : pathToExtension.entrySet()) {
String path = entry.getKey();
Extension e = entry.getValue();
String url = e.getUrl();
if (isAbsolute(url)) {
// Option A: find all versions of the extension and pass validation if the instance conforms to at least one
// Option B: introspect the existing constraints and only add this one if the extension is not covered by profile constraints
// remove the array indices from the path
String reducedPath = path.replaceAll("\\[[0-9]+\\]", "");

// Option A: conformance to any one version of the extension is sufficient
if (profileVersions.containsKey(url)) {
String constraint = profileVersions.get(url).stream()
.map(v -> "conformsTo('" + url + "|" + v + "')")
.collect(Collectors.joining(" or "));
// if the profile validation already has this covered, then move on to the next extension
if (alreadyGeneratedExtensionConstraints.containsKey(reducedPath)
&& alreadyGeneratedExtensionConstraints.get(reducedPath).contains(url)) {
continue;
}

constraints.add(Constraint.Factory.createConstraint("generated-ext-1", Constraint.LEVEL_RULE, path,
"Extension must conform to at least one definition of '" + url + "'", constraint, SOURCE_VALIDATOR, false, true));
// otherwise generate the instance-specifc conformsTo constraint (or add a warning if we don't have a definition for it)
if (FHIRRegistry.getInstance().hasResource(url, StructureDefinition.class)) {
constraints.add(Constraint.Factory.createConstraint("generated-ext~" + path + "<" + url + ">", Constraint.LEVEL_RULE, path,
"Extension must conform to definition '" + url + "'",
"conformsTo('" + url + "')", SOURCE_VALIDATOR, false, true));
} else {
issues.add(issue(IssueSeverity.WARNING, IssueType.NOT_SUPPORTED, "Extension definition '" + url + "' is not supported", null, path));
}

// Option B: conditionally add a conformsTo constraint for the default/latest version of this extension
// if (FHIRRegistry.getInstance().hasResource(url, StructureDefinition.class)) {
// constraints.add(Constraint.Factory.createConstraint("generated-ext-2", Constraint.LEVEL_RULE, path,
// "Extension must conform to definition '" + url + "'",
// "conformsTo('" + url + "')", SOURCE_VALIDATOR, false, true));
// } else {
// issues.add(issue(IssueSeverity.WARNING, IssueType.NOT_SUPPORTED, "Extension definition '" + url + "' is not supported", null, path));
// }
}
}

validate(resourceNode, constraints);
}

private Map<String, Set<String>> collectProfileVersions(Collection<Extension> extensions) {
Map<String, Set<String>> profileVersions = new HashMap<>();
/**
* Collect all the extensions from the resource instance and index them by their simple FHIRPath paths
*/
private Map<String, Extension> gatherInstanceExtensions(FHIRPathResourceNode resourceNode) {
PathAwareCollectingVisitor<Extension> extCollector = new PathAwareCollectingVisitor<Extension>(Extension.class);
resourceNode.resource().accept(extCollector);
Map<String, Extension> pathToExtension = extCollector.getResult();
return pathToExtension;
}

Set<String> uniqueExtensionUrls = extensions.stream()
.map(e -> e.getUrl())
.distinct()
.collect(Collectors.toSet());
/**
* Gather all the constraint ids and process the specially-marked extension ones into a map
* from the paths to the set of corresponding extension URLs (with no version suffix).
*/
private Map<String, Set<String>> gatherExtensionConstraints(List<Constraint> constraints) {
Map<String, Set<String>> alreadyGeneratedExtensionConstraints = new HashMap<>();
for (Constraint c : constraints) {
int urlMark = c.id().lastIndexOf("<");
if (urlMark <= 0 || !c.id().endsWith(">")) {
continue;
}

// RegistryResourceProviders have a method thats basically just what we want, but unfortunately the registry does not
for (FHIRRegistryResource rr : FHIRRegistry.getInstance().getRegistryResources(StructureDefinition.class)) {
if (uniqueExtensionUrls.contains(rr.getUrl()) && rr.getVersion() != null) {
profileVersions.computeIfAbsent(rr.getUrl(), x -> new HashSet<>()).add(rr.getVersion().toString());
String firstPart = c.id().substring(0, urlMark);
int pathMark = firstPart.lastIndexOf("~");
if (pathMark <= 0) {
continue;
}
}
String path = firstPart.substring(pathMark + 1);

return profileVersions;
String url = c.id().substring(urlMark + 1, c.id().length() - 1);
alreadyGeneratedExtensionConstraints.computeIfAbsent(path, t -> new HashSet<>()).add(url);
}
return alreadyGeneratedExtensionConstraints;
}

private void validateProfileReferences(FHIRPathResourceNode resourceNode, List<String> profiles, boolean resourceAsserted) {
Expand Down

0 comments on commit 4d74d9f

Please sign in to comment.