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

FM2-613: Upgrade Hapi Fhir version to v5.7.9 #521

Merged
merged 6 commits into from
Oct 30, 2023

Conversation

mherman22
Copy link
Member

@mherman22 mherman22 commented Sep 3, 2023

Description of what I changed

https://github.com/hapifhir/org.hl7.fhir.core/tree/master/org.hl7.fhir.convertors

Issue I worked on

see https://issues.openmrs.org/browse/FM2-613

Checklist: I completed these to help reviewers :)

  • My IDE is configured to follow the code style of this project.

    No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend

  • I have added tests to cover my changes. (If you refactored
    existing code that was well tested you do not have to add tests)

    No? -> write tests and add them to this commit git add . && git commit --amend

  • I ran mvn clean package right before creating this pull request and
    added all formatting changes to my commit.

    No? -> execute above command

  • All new and existing tests passed.

    No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.

  • My pull request is based on the latest changes of the master branch.

    No? Unsure? -> execute command git pull --rebase upstream master

@mherman22 mherman22 marked this pull request as ready for review September 3, 2023 20:08
Copy link
Member Author

@mherman22 mherman22 left a comment

Choose a reason for hiding this comment

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

There is need to create a custom class that inherits the VersionConvertorFactory_30_40 class since in this version of Hapi Fhir, accessing convertion class is no longer possible.

Quoted from here!-> Note that these classes are not intended to be used directly. When actually converting resources, the provided conversion factory classes are intended to be used as the entry point. For example, to convert a dstu3 AllergyIntolerance resource, the above conversion would not use AllergyIntolerance40_50 directly, but would instead call: VersionConvertorFactory_40_50.convertResource(dstu3AllergyIntolerance). VersionConvertorFactory_40_50 would call AllergyIntolerance40_50 internally to convert r4AllergyIntolerance.

cc: @ibacher

@mherman22
Copy link
Member Author

There is need to create a custom class that inherits the VersionConvertorFactory_30_40 class since in this version of Hapi Fhir, accessing convertion class is no longer possible.

Quoted from here!-> Note that these classes are not intended to be used directly. When actually converting resources, the provided conversion factory classes are intended to be used as the entry point. For example, to convert a dstu3 AllergyIntolerance resource, the above conversion would not use AllergyIntolerance40_50 directly, but would instead call: VersionConvertorFactory_40_50.convertResource(dstu3AllergyIntolerance). VersionConvertorFactory_40_50 would call AllergyIntolerance40_50 internally to convert r4AllergyIntolerance.

cc: @ibacher

which explains the failing test on Tasks

@ibacher ibacher self-requested a review September 8, 2023 20:28
@ibacher
Copy link
Member

ibacher commented Sep 8, 2023

@mherman22 Sorry, I missed this PR until now. Yep, that looks to be a problem. I'm going to try a couple of things to see if we can avoid doing that, since it sounds like a lot of code.

@ibacher
Copy link
Member

ibacher commented Sep 11, 2023

Well, we've got a new error now, I guess. Not sure if HAPI moved where the Pointcut definitions are stored?

@mherman22
Copy link
Member Author

Well, we've got a new error now, I guess. Not sure if HAPI moved where the Pointcut definitions are stored?

Will look into it tonight

@mherman22
Copy link
Member Author

These very tests were failing in intellij and when i ran mvn clean package in the terminal, they were still failing. However, when i ran mvn clean install in the terminal, it was all successful and it seemed to install so many artifacts from the new version(v5.7.9). so when i triggered mvn clean package again, everything passes and in the IDE aswell.

So, the PointCut class didnt really change that much in the version(5.7.9) -> https://github.com/hapifhir/hapi-fhir/blob/v5.7.9/hapi-fhir-base/src/main/java/ca/uhn/fhir/interceptor/api/Pointcut.java

FYI: i was following the conversation at hapifhir/hapi-fhir#316 (comment), especially that comment.

cc: @ibacher

@ibacher
Copy link
Member

ibacher commented Sep 12, 2023

@mherman22 Are you running mvn clean install -P integration-test or similar?

@mherman22
Copy link
Member Author

@mherman22 Are you running mvn clean install -P integration-test or similar?

I guess I should run that and report back.

@ibacher
Copy link
Member

ibacher commented Sep 12, 2023

So, it looks like this problem arises because of the version of Jackson we're using. I don't see any easy way forward that will continue to support older versions of OpenMRS.

In any case, we were going to dump support for pre-2.4.x versions at some point, so I guess that's now a prerequisite for this.

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (6b00ddb) 77.79% compared to head (82b3441) 77.82%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #521      +/-   ##
============================================
+ Coverage     77.79%   77.82%   +0.03%     
+ Complexity     2679     2678       -1     
============================================
  Files           239      239              
  Lines          7430     7440      +10     
  Branches        897      897              
============================================
+ Hits           5780     5790      +10     
  Misses         1114     1114              
  Partials        536      536              
Files Coverage Δ
...api/search/SearchQueryBundleProviderR3Wrapper.java 57.89% <100.00%> (ø)
...ers/r3/AllergyIntoleranceFhirResourceProvider.java 91.30% <100.00%> (+0.40%) ⬆️
...r2/providers/r3/ConditionFhirResourceProvider.java 77.27% <100.00%> (+1.08%) ⬆️
...iders/r3/DiagnosticReportFhirResourceProvider.java 91.30% <100.00%> (+0.40%) ⬆️
...r2/providers/r3/EncounterFhirResourceProvider.java 38.98% <100.00%> (+1.05%) ⬆️
.../fhir2/providers/r3/GroupFhirResourceProvider.java 88.24% <100.00%> (+1.57%) ⬆️
...ir2/providers/r3/LocationFhirResourceProvider.java 90.91% <100.00%> (+0.43%) ⬆️
...ers/r3/MedicationDispenseFhirResourceProvider.java 92.59% <100.00%> (ø)
...ders/r3/MedicationRequestFhirResourceProvider.java 80.00% <100.00%> (ø)
.../providers/r3/ObservationFhirResourceProvider.java 90.62% <100.00%> (ø)
... and 7 more

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

@ibacher
Copy link
Member

ibacher commented Oct 30, 2023

@mherman22 Would you be able to take care of getting this PR to work with the new 2.0.0-SNAPSHOT version?

@mherman22
Copy link
Member Author

@mherman22 Would you be able to take care of getting this PR to work with the new 2.0.0-SNAPSHOT version?

Alright, lemi do that

@mherman22
Copy link
Member Author

@ibacher you can take a look.

@ibacher ibacher merged commit 6196a97 into openmrs:master Oct 30, 2023
4 checks passed
@ibacher
Copy link
Member

ibacher commented Oct 30, 2023

Nice work @mherman22!

@mherman22 mherman22 deleted the FM2-613 branch October 31, 2023 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants