Feature Request: Add client support for $process-message special oper… #542

Merged
merged 6 commits into from Mar 17, 2017

Conversation

Projects
None yet
2 participants
@hugosoares
Contributor

hugosoares commented Jan 18, 2017

…ation #407

Changed client to send messages to a processing server:
Object response = client
.operation()
.onServer()
.processMessage() // New operation for sending messages
.setResponseUrlParam("http://myserver/fhir") //optional url parameters to send with the POST
.setAsyncProcessingMode()//optional url parameters to send with the POST
.setMessageBundle(msgBundle) // the Message bundle to send
.execute();

hugosoares added some commits Jan 18, 2017

Feature Request: Add client support for $process-message special oper…
…ation #407

Changed client to send messages to a processing server:
Object response = client
.operation()
.onServer()
.processMessage()
.setResponseUrlParam("http://myserver/fhir")
.setMessageBundle(msgBundle)
.execute();
@hugosoares

This comment has been minimized.

Show comment
Hide comment
@hugosoares

hugosoares Jan 20, 2017

Contributor

Closing an reopen to trigger travis

Contributor

hugosoares commented Jan 20, 2017

Closing an reopen to trigger travis

@hugosoares hugosoares closed this Jan 20, 2017

@hugosoares hugosoares reopened this Jan 20, 2017

@jamesagnew

Hi Hugo,

Sorry to take so long to review this, I needed to educate myself a bit on how $process-message actually works before I felt qualified to review :)

I think this looks awesome, and I'm looking forward to merging. I have a few comments on the design, definitely nothing major, but would love to your your thoughts on them.

@@ -76,6 +76,8 @@
private Boolean myPrettyPrint = false;
private SummaryEnum mySummary;
private final String myUrlBase;
+ private Boolean myAsync = null;

This comment has been minimized.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

I'm wondering if it might make more sense to not make these things global properties of the client, and instead make them in OperationMethodBinding#createProcessMsgInvocation?

I think that would be a bit more consistent with our general approach of making settings which are specific to a particular type of operation be non-global.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

I'm wondering if it might make more sense to not make these things global properties of the client, and instead make them in OperationMethodBinding#createProcessMsgInvocation?

I think that would be a bit more consistent with our general approach of making settings which are specific to a particular type of operation be non-global.

This comment has been minimized.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Makes perfect sense. I Will make that change.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Makes perfect sense. I Will make that change.

@@ -24,5 +24,6 @@
public interface IOperationUnnamed {
IOperationUntyped named(String theName);
+ IOperationProcessMsg processMessage();

This comment has been minimized.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

I'm thinking a javadoc link to the messaging part of the FHIR spec would be a useful addition here if possible

@jamesagnew

jamesagnew Jan 31, 2017

Owner

I'm thinking a javadoc link to the messaging part of the FHIR spec would be a useful addition here if possible

This comment has been minimized.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

Actually, I'm also wondering if it might make sense to move this method from IOperationUnnamed over to IOperation.

Technically with it on IOperationUnnamed you could invoke this operation at the type or instance level, but FHIR doesn't allow that so the API should probably try to prevent it too.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

Actually, I'm also wondering if it might make sense to move this method from IOperationUnnamed over to IOperation.

Technically with it on IOperationUnnamed you could invoke this operation at the type or instance level, but FHIR doesn't allow that so the API should probably try to prevent it too.

This comment has been minimized.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Makes perfect sense. I Will make that change.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Makes perfect sense. I Will make that change.

+ *
+ * @author HGS
+ */
+public interface IOperationProcessMsg<T extends IBaseResource> extends IClientExecutable<IOperationProcessMsg<T>, T> {

This comment has been minimized.

@jamesagnew

jamesagnew Jan 31, 2017

Owner

You could probably add typing here, in order for client code to avoid the need to cast the result of the execute() method later.

E.g. add one more interface to the chain so that client calls look like:

Bundle response = client
       .operation()
       .onServer()
       .processMessage()
       .setResponseUrlParam("http://myserver/fhir")
       .setMessageBundle(msgBundle)
       .synchronous(Bundle.class)
       .execute();

...and....

OperationOutcome response = client
       .operation()
       .onServer()
       .processMessage()
       .setResponseUrlParam("http://myserver/fhir")
       .setMessageBundle(msgBundle)
       .asynchronous(OperationOutcome.class)
       .execute();
@jamesagnew

jamesagnew Jan 31, 2017

Owner

You could probably add typing here, in order for client code to avoid the need to cast the result of the execute() method later.

E.g. add one more interface to the chain so that client calls look like:

Bundle response = client
       .operation()
       .onServer()
       .processMessage()
       .setResponseUrlParam("http://myserver/fhir")
       .setMessageBundle(msgBundle)
       .synchronous(Bundle.class)
       .execute();

...and....

OperationOutcome response = client
       .operation()
       .onServer()
       .processMessage()
       .setResponseUrlParam("http://myserver/fhir")
       .setMessageBundle(msgBundle)
       .asynchronous(OperationOutcome.class)
       .execute();

This comment has been minimized.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Nice tip. I Will make that change.

@hugosoares

hugosoares Jan 31, 2017

Contributor

Nice tip. I Will make that change.

hugosoares added some commits Feb 9, 2017

@hugosoares

This comment has been minimized.

Show comment
Hide comment
@hugosoares

hugosoares Feb 9, 2017

Contributor

Hi James,
I just commited the changes requested.
Can you merge this in time for the next release?
We have some clients in need of this feature that are going live soon. It would be great the could upgrade their hapi lib and use this instead of an ugly work around they use right now.

Contributor

hugosoares commented Feb 9, 2017

Hi James,
I just commited the changes requested.
Can you merge this in time for the next release?
We have some clients in need of this feature that are going live soon. It would be great the could upgrade their hapi lib and use this instead of an ugly work around they use right now.

@hugosoares hugosoares closed this Feb 9, 2017

@jamesagnew jamesagnew reopened this Feb 10, 2017

@jamesagnew jamesagnew merged commit da9daf8 into jamesagnew:master Mar 17, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@jamesagnew

This comment has been minimized.

Show comment
Hide comment
@jamesagnew

jamesagnew Mar 17, 2017

Owner

Hi Hugo,

This has now been merged. Thanks again for the contribution!

Owner

jamesagnew commented Mar 17, 2017

Hi Hugo,

This has now been merged. Thanks again for the contribution!

jamesagnew added a commit that referenced this pull request Mar 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment