Implement CTerminologyCode.constraint as String#761
Implement CTerminologyCode.constraint as String#761MattijsK wants to merge 8 commits into670_feature_archie_4_major_versionfrom
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 670_feature_archie_4_major_version #761 +/- ##
========================================================================
+ Coverage 72.34% 72.38% +0.03%
- Complexity 7189 7193 +4
========================================================================
Files 678 678
Lines 23267 23299 +32
Branches 3764 3786 +22
========================================================================
+ Hits 16833 16864 +31
+ Misses 4707 4704 -3
- Partials 1727 1731 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| public abstract void setAssumedValue(ValueType assumedValue); | ||
|
|
||
| public abstract List<Constraint> getConstraint(); | ||
| public abstract Object getConstraint(); |
There was a problem hiding this comment.
I think it should be possible to do:
| public abstract Object getConstraint(); | |
| public abstract Constraint getConstraint(); |
Meaning that when classes extend CPrimitiveObject, they have to edit the type of constraint, for example CBoolean has to do:
public class CBoolean extends CPrimitiveObject<List<Boolean>, Boolean>
Then we also don't need the convenience method getConstraintAsList, as getConstraint will return a String for CTerminologyCode, but a List for CBoolean.
addConstraint(Constraint constraint) will have to be removed from CPrimitiveObject, but can then be added as a convenience method to objects that have a list as constraint, like CBoolean.
For me this feels a bit more like how the specifications intended it, instead of a little workaround.
There was a problem hiding this comment.
I changed the type here back to Constraint. Can you check if it makes more sense now?
I still kept getConstraintAsList in though.
| @Deprecated | ||
| public boolean isValidValue(TerminologyCode value) { | ||
| if(getConstraint().isEmpty()) { | ||
| if(getConstraint() == null || getConstraint().isEmpty()) { |
There was a problem hiding this comment.
I feel the functionality has changed here, because previously if the constraint list contained an empty string this if statement wasn't true and now it is. And also further in the code I don't see any reason why a getConstraint() of an empty string would return true by default.
| List<String> otherValueSet = otherCode.getValueSetExpanded(); | ||
|
|
||
| if(constraint.size() != 1) { | ||
| // TODO: does this need to be removed/reworded or anything else? |
There was a problem hiding this comment.
Yes, I think this should be reworded. I don't see any specific wording for this in the specifications, so I guess we can just change the wording?
There was a problem hiding this comment.
I think these checks are completely changed now and the objects would actually conform to if the constraint of the parent or itself are null now. I changed this, please check if it makes sense to you.
| @Override | ||
| public void serialize(CTerminologyCode cobj) { | ||
| if (!cobj.getConstraint().isEmpty()) { | ||
| if (cobj.getConstraint() != null && !cobj.getConstraint().isEmpty()) { |
There was a problem hiding this comment.
| if (cobj.getConstraint() != null && !cobj.getConstraint().isEmpty()) { | |
| if (cobj.getConstraint() != null) { |
I think empty strings would just be used in the code previously, so I think we shouldn't change that.
| } else if (cObject instanceof CPrimitiveObject) { | ||
| CPrimitiveObject<?, ?> cPrimitiveObject = (CPrimitiveObject<?, ?>) cObject; | ||
| List<?> constraint = cPrimitiveObject.getConstraint(); | ||
| } else if (cObject instanceof COrdered) { |
There was a problem hiding this comment.
If we keep the getConstraintAsList() it is maybe better to use that instead, in the case (that will probably never happen) that a new CPrimitiveObject based on an Interval constraint is introduced.
There was a problem hiding this comment.
I think I can keep it the way it is, I added a clarifying comment though.
|
|
||
| private static RMObjectValidationMessage createValidationMessage(Object value, String pathSoFar, CPrimitiveObject<?, ?> cobject) { | ||
| List<?> constraint = cobject.getConstraint(); | ||
| List<?> constraint = cobject.getConstraintAsList(); |
There was a problem hiding this comment.
Or maybe just use getConstraint() and instead of checking if the constraint.size() == 1, check if the constraint is a list (and maybe has more than 1 element).
There was a problem hiding this comment.
I don't think this makes much sense. I still kept in getConstraintAsList, so this keeps it the simplest.
|
|
||
| private boolean isValidValue(CTerminologyCode terminologyCode, TerminologyCode value) { | ||
| if(terminologyCode.getConstraint().isEmpty()) { | ||
| if(terminologyCode.getConstraint() == null || terminologyCode.getConstraint().isEmpty()) { |
There was a problem hiding this comment.
| if(terminologyCode.getConstraint() == null || terminologyCode.getConstraint().isEmpty()) { | |
| if(terminologyCode.getConstraint() == null |
Again I don't think previously an empty string would by default return true.
Make it Constraint again instead of Object
| public abstract List<Constraint> getConstraint(); | ||
| public abstract Constraint getConstraint(); | ||
|
|
||
| public abstract void setConstraint(List<Constraint> constraint); |
There was a problem hiding this comment.
This abstract method can stay, but just with the parameter Constraint instead of List
| /** Temporary storage for multi-code ADL 1.4 constraints during conversion. Never serialized. */ | ||
| @JsonIgnore | ||
| @XmlTransient | ||
| private List<String> pendingCodes; |
There was a problem hiding this comment.
When parsing a ADL1.4 archetype, the codes will end up here. But when you try and serialize the archetype again, the constraint of the defining code now stays empty.
There was a problem hiding this comment.
So maybe we should add in the serializer that the pendingCodes should be converted into a list of strings for the constraint. Or we should maybe have two separate CTerminologyCode classes. One for ADL1.4 and one for ADL2.
Summary
This PR fixes #643. As there was a difference between the openEHR specification and Archie. So it changes
CTerminologyCode.constraintfromList<String>toString.Changes
Core model (
CTerminologyCode)constraintfield changed fromList<String>toString;getConstraint()now returnsStringaddConstraint(String)removed;setConstraint(String)takes a single valuegetConstraintAsList()added as a convenience method for call sites that need list semantics,returning a singleton list or an empty list
Abstract primitive object interface (
CPrimitiveObject)getConstraint()return type generalised toObjectto accommodate the now-divergent constrainttypes across subclasses
getConstraintAsList()added as abstract, implemented in all subclasses; annotated@JsonIgnoreso it is not serialised as a separate property
addConstraint()removed from the abstract interfaceADL 1.4 → ADL 2 conversion (
ADL14TermConstraintConverter,Adl14PrimitivesConstraintParser)ADL 1.4 allows inline multi-code constraints (e.g.
[local::at0001, at0002]and[snomed-ct::12345, 67890]) which have no direct ADL 2 equivalent. These are handled in atwo-phase approach:
@JsonIgnore @XmlTransient List<String> pendingCodesfield on
CTerminologyCode— used only during conversion, never serialisedpendingCodesand creates a proper value set (ac-code) from them, settingthe resulting ac-code as the single
constraint[terminology::code]) bythe parser so the converter can treat all pending codes uniformly
convertCTerminologyCode(single-code),convertMultiCodeTerminologyCode(multi-code), andconvertAssumedValue(shared assumed-valuelogic)
Serialisation
"constraint": ["at1"]) is handled by the existingUNWRAP_SINGLE_VALUE_ARRAYSJackson feature