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

Subscriptions correctly interpreting the payload #1357

Merged
merged 4 commits into from Aug 14, 2019

Conversation

@seanmcilvenna
Copy link
Contributor

commented Jun 21, 2019

Currently the subscription framework in HAPI-FHIR does not correctly interpret the meaning of the Subscription.channel.payload property. The payload property is described by the FHIR spec as:

The mime type to send the payload in - either application/fhir+xml, or application/fhir+json. If the payload is not present, then there is no payload in the notification, just a notification. The mime type "text/plain" may also be used for Email and SMS subscriptions.

Right now, HAPI-FHIR simply sends the raw text value of the Subscription.channel.payload property, rather than using the value of the payload property to determine how (or whether) to serialize the resource that triggered the subscription in the body of the notification (whether that be a rest-hook subscription or an email subscription).

This changes make it so that it HAPI tests the value of the Subscription.channel.payload and serializes the resource that triggered the subscription in the body of the notification, which should align with the FHIR specification's intent.

Changes to subscriptions to include the resource that triggered the s…
…ubscription in the payload, serialized as JSON or XML depending on the Subscription.channel.payload property's value
@jamesagnew
Copy link
Owner

left a comment

This looks great! One stylistic change I'd recommend but otherwise no objections.

@@ -89,9 +97,15 @@ public void setSubscription(CanonicalSubscription theSubscription) {
mySubscription = theSubscription;
}

public void setPayload(FhirContext theCtx, IBaseResource thePayload) {
public void setPayload(FhirContext theCtx, IBaseResource thePayload, Boolean isXml) {

This comment has been minimized.

Copy link
@jamesagnew

jamesagnew Jun 21, 2019

Owner

s/isXml/theXml/ for style consistency.

This comment has been minimized.

Copy link
@pandapaul

pandapaul Jul 13, 2019

isXml makes more sense for a Boolean, but I might be misunderstanding.

This comment has been minimized.

Copy link
@seanmcilvenna

seanmcilvenna Aug 9, 2019

Author Contributor

This is a matter of preference. This is a boolean, so I prefer "isXml" as does @pandapaul... But, if you require that I change it to get the PR merged, I will :) Let me know @jamesagnew

This comment has been minimized.

Copy link
@jamesagnew

jamesagnew Aug 14, 2019

Owner

Much of the HAPI FHIR codebase uses the code style of:

  • Parameters are named theFoo
  • Fields are named myFoo
  • Statics are named ourFoo
  • Constants are named FOO_CONSTANT

We aren't strict on this at all, but I do think it makes sense to match the existing style of a class when adding to it.

This comment has been minimized.

Copy link
@seanmcilvenna

seanmcilvenna Aug 14, 2019

Author Contributor

Will update to theXml in just a sec

@jamesagnew jamesagnew merged commit 6ecaa4f into jamesagnew:master Aug 14, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
jamesagnew added a commit that referenced this pull request Aug 14, 2019

@seanmcilvenna seanmcilvenna deleted the lantanagroup:subscription-payload branch Aug 14, 2019

@jamesagnew

This comment has been minimized.

Copy link
Owner

commented Aug 15, 2019

FYI- This unfortunately seems to have caused a bunch of downstream regressions in other projects that were depending on the serialized resource being in the delivery message (i.e. the one in the queue, in ResourceDeliveryMessage) even if no payload type was specified. I'm going to see if I can tweak this to keep the right behaviour without dropping that functionality.

jamesagnew added a commit that referenced this pull request Aug 15, 2019
jamesagnew added a commit that referenced this pull request Aug 15, 2019
jamesagnew added a commit that referenced this pull request Aug 15, 2019
Fix an unintended regression in #1357 (#1429)
* Start working on a tweak to #1357 - Not yet complete

* Tweaks to avoid an unintended regression from #1357
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.