Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dependent rules with paths that must not exist may evaluate on outdated rmObject #602

Merged
merged 6 commits into from
Jun 14, 2024

Conversation

EBrader
Copy link
Collaborator

@EBrader EBrader commented May 30, 2024

Create PathsThatMustNotExistFixer with similar purpose as for AssertionFixer that removes pathsThatMustNotExist from the root asserted during rule evaluation so that next evaluated rules take non-existence into account.

Fix applies only for paths that must not exist referring to object instance of Pathable with parent being any instance of ItemStructure, Cluster, Composition or Section. This limited set is the result of needing to know the actual type of the parent in order to call relevant methods to either delete the child from a list or set a value to null. If the desire is there to support all types, almost for all existing RMObject types, a check and cast would be needed.

To be able to import the objects from project openehr-rm, testImplementation needed to be changed to api in build.gradle of tools.

… from evaluated root so that followup rules take that into account
@EBrader EBrader requested review from MattijsK and J3173 May 30, 2024 13:23
@EBrader EBrader self-assigned this May 30, 2024
Copy link

codecov bot commented May 30, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 10 lines in your changes missing coverage. Please review.

Project coverage is 71.80%. Comparing base (c065910) to head (711d751).
Report is 1 commits behind head on master.

Files Patch % Lines
...nedap/archie/rules/evaluation/AssertionsFixer.java 82.45% 3 Missing and 7 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #602      +/-   ##
============================================
- Coverage     71.80%   71.80%   -0.01%     
- Complexity     6956     6976      +20     
============================================
  Files           663      663              
  Lines         22691    22757      +66     
  Branches       3676     3682       +6     
============================================
+ Hits          16294    16341      +47     
- Misses         4664     4686      +22     
+ Partials       1733     1730       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

J3173
J3173 previously requested changes Jun 3, 2024
Comment on lines 17 to 19
public void fixPathsThatMustNotExist(AssertionResult assertionResult) {
assertionResult.getPathsThatMustNotExist().forEach(this::removePathThatMustNotExist);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should first collect all the objects that need to be removed, before actually removing them. Otherwise it won't work when multiple things need to be removed from the same list. E.g. multiple 'id10's in your test archetype.

See for example the processAssertionResults method in our internal healthcare-ehr-components library.

Comment on lines 23 to 30
if (match instanceof Pathable) {
Pathable item = (Pathable) match;
Pathable parent = item.getParent();
if (parent instanceof ItemStructure) ((ItemStructure) parent).getItems().remove(item);
else if (parent instanceof Cluster) ((Cluster) parent).getItems().remove(item);
else if (parent instanceof Composition) ((Composition) parent).getContent().remove(item);
else if (parent instanceof Section) ((Section) parent).getItems().remove(item);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should use reflections / ModelInfoLookup to make this work for all classes and attributes. We can discuss that offline.

testImplementation project(':openehr-rm')
api project(':openehr-rm')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use reflections (see other comment), this should not be necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, this dependency not being here is by design, so we can switch to other RM implementations. So, reflection, or some kind of plugin in the model info lookup is the way to go, instead of this.

@EBrader EBrader requested a review from J3173 June 6, 2024 15:00
Copy link
Collaborator

@J3173 J3173 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main code looks good. @MattijsK Can you take a look at the tests?

@J3173 J3173 dismissed their stale review June 11, 2024 08:54

The requested changes have been applied

Copy link
Collaborator

@MattijsK MattijsK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice clear test, seems all good to me!

@EBrader EBrader merged commit 0853246 into master Jun 14, 2024
4 checks passed
@EBrader EBrader deleted the 3867 branch June 14, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants