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

Issue-1077: Make CapabilityStatementProvider use the closest common s… #1088

Merged
merged 1 commit into from Nov 1, 2019

Conversation

@srdo
Copy link
Contributor

srdo commented Oct 3, 2018

…uperclass of provided resources when generating rest.resource.profile, instead of always using the base definition.

This resolves #1077.

The capabilitystatement.rest.resource.profile link should point to a StructureDefinition that applies to all profiles of that particular resource name. The current implementation simply points to the base definitions for the name, since these profiles are guaranteed to be adhered to by profiles extending the base definitions.

The changes here try to narrow down the right StructureDefinition link further. For example, if a server only provides a Patient profiling called MyPatient, there's no reason not to point to the MyPatient StructureDefinition in capabilitystatement.rest.resource.profile.

The approach here is to look at all the resource providers for a given resource name, and find the "most specific" resource superclass that is a supertype of all profiles specified via getResourceType. The profile for that type should apply to all subclasses as well, so it's the most specific profile that still applies to all resources with that name on the server.

For example, say the hierarchy for Patient is

Patient -> MyPatient -> MyPatient2
                     -> MyPatient3

If the server has two resource providers for Patient, with one supplying MyPatient2 and one supplying MyPatient3, we can see that the common supertypes for these are Patient and MyPatient. Since MyPatient is the most specific profiling that still applies to all the provided resources, it should be the one linked to in capabilitystatement.rest.resource.profile.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 553cdd6 to 6e95452 Oct 3, 2018
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Oct 3, 2018

I'm happy to make the same changes to the R4 capability statement provider, but I'd like feedback on this before I spend more time on it.

@srdo srdo force-pushed the srdo:issue-1077-3 branch 5 times, most recently from 949567d to 99bde61 Jan 4, 2019
@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented May 30, 2019

Hi @srdo - Apologies for the slow review on this. The approach makes lots of sense to me. If you're interested in making the equivalent R4 change I'm definitely happy to merge.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 99bde61 to 77dbe25 May 30, 2019
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented May 30, 2019

@jamesagnew Rebased and added R4 support. Please let me know when you've reviewed, and I will squash to one commit.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 77dbe25 to 32e2eb7 Jul 9, 2019
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Jul 9, 2019

Rebased to fix conflicts.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 32e2eb7 to fbbb24e Jul 15, 2019
@srdo srdo force-pushed the srdo:issue-1077-3 branch from fbbb24e to 2e229aa Aug 14, 2019
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Aug 19, 2019

@jamesagnew Friendly reminder, this should be ready to merge once you have time to look at it.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 2e229aa to 8b1952f Aug 23, 2019
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Aug 23, 2019

Implemented this for R5 as well, also switched all tests to use the non-deprecated ServerCapabilityProvider constructor.

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 8b1952f to bc51b69 Oct 14, 2019
@lgtm-com

This comment has been minimized.

Copy link

lgtm-com bot commented Oct 14, 2019

This pull request introduces 1 alert when merging bc51b69 into 92c6b88 - view on LGTM.com

new alerts:

  • 1 for Container contents are never accessed
@srdo srdo force-pushed the srdo:issue-1077-3 branch 4 times, most recently from feb0ee6 to 8f65d21 Oct 14, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 16, 2019

Codecov Report

Merging #1088 into master will decrease coverage by 0.46%.
The diff coverage is 91.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1088      +/-   ##
============================================
- Coverage     75.64%   75.17%   -0.47%     
+ Complexity    12891    12707     -184     
============================================
  Files           927      924       -3     
  Lines         52272    52093     -179     
  Branches       8743     8716      -27     
============================================
- Hits          39539    39160     -379     
- Misses         9274     9451     +177     
- Partials       3459     3482      +23
Impacted Files Coverage Δ Complexity Δ
...rest/server/ServerCapabilityStatementProvider.java 75.26% <100%> (-1.8%) 51 <0> (-1)
...rest/server/ServerCapabilityStatementProvider.java 80.76% <100%> (-0.69%) 60 <0> (+1)
...rest/server/ServerCapabilityStatementProvider.java 77.18% <100%> (+0.28%) 65 <6> (+1) ⬆️
...in/java/ca/uhn/fhir/rest/server/RestfulServer.java 78.83% <60%> (-2.6%) 189 <0> (-8)
...n/fhir/rest/server/RestfulServerConfiguration.java 91.22% <89.47%> (-0.36%) 43 <8> (+8)
...ir/rest/server/CommonResourceSupertypeScanner.java 94.73% <94.73%> (ø) 9 <9> (?)
...fhir/jpa/provider/r5/JpaConformanceProviderR5.java 20.25% <0%> (-58.76%) 5% <0%> (-14%)
.../java/ca/uhn/fhir/jpa/model/util/JpaConstants.java 50% <0%> (-50%) 1% <0%> (ø)
...n/fhir/jpa/config/HapiFhirHibernateJpaDialect.java 54.16% <0%> (-37.5%) 7% <0%> (-5%)
...module/subscriber/ResourceModifiedJsonMessage.java 36.36% <0%> (-27.28%) 2% <0%> (-1%)
... and 109 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97d5edc...8bd5254. Read the comment docs.

Copy link
Contributor

jvitrifork left a comment

Looks good

@srdo srdo force-pushed the srdo:issue-1077-3 branch from 8f65d21 to 16cd37d Oct 31, 2019
…uperclass of provided resources when generating rest.resource.profile, instead of always using the base definition.
@srdo srdo force-pushed the srdo:issue-1077-3 branch from 16cd37d to 8bd5254 Oct 31, 2019
@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Oct 31, 2019

@jvitrifork If this looks fine, could you merge it? I'd like to not have to keep fixing conflicts with master.

@jkiddo

This comment has been minimized.

Copy link
Contributor

jkiddo commented Oct 31, 2019

@srdo Im afraid that my priviledges are the same as yours. You need the attention of @jamesagnew I guess ;)

@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Oct 31, 2019

Ah, sorry. Assumed that you had merge access due to the approval.

@jkiddo

This comment has been minimized.

Copy link
Contributor

jkiddo commented Oct 31, 2019

My hope was that 'my approval' could help push this on its way

(@jvitrifork == @jkiddo)

@jamesagnew jamesagnew merged commit 80bfb9a into jamesagnew:master Nov 1, 2019
2 of 3 checks passed
2 of 3 checks passed
jamesagnew.hapi-fhir Build #20191031.7 had test failures
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Java No new or fixed alerts
Details
@jamesagnew

This comment has been minimized.

Copy link
Owner

jamesagnew commented Nov 1, 2019

Sorry for the slow merge... We've been backlogged while trying to get infrastructure issues solved (CI and coverage, etc.). Now that we're past that, it's time for digging out!

@srdo

This comment has been minimized.

Copy link
Contributor Author

srdo commented Nov 1, 2019

Thanks

jamesagnew added a commit that referenced this pull request Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.