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

Enhance HAPI RestfulServer to support adding resource provider as CDI bean in JBoss enviroment #291

Closed
fw060 opened this Issue Feb 3, 2016 · 9 comments

Comments

Projects
None yet
2 participants
@fw060

fw060 commented Feb 3, 2016

We have encountered an issue in RestfulServer.bindMethod when we trying to add a resource provider using CDI in JBoss Wildfly environment (see code below).

In this setup, the resource provider passed in to RestfulServer is not PatientProvider class, but a proxy class generated by JBoss. Then bindMethod would have trouble inspect getPatient method in PatientProvider. After some investigating, we find out that this is caused by the issue that Jboss Proxy loses generic type information (see https://issues.jboss.org/browse/WELD-1539). This will lead to error when bindMethod trying to inspect getPatient method which would return a List of Patient. bindMethod can only see List without any generic type so a null pointer exception would happen.

This is NOT an issue in HAPI, however, maybe solving this issue in HAPI is the cleanest way since other CDI environment might have similar problem.

Our current thought is to create a new method:
public void addResourceProvider(IResourceProvider theResourceProvider, Class theProviderClass)
which would allow designating the Provider class rather than deceiving provider class from the provider instance itself.

I will be more than happy to provide any more detailed information. If anyone has a better/cleaner solution, please also let us know. I will provider the pull request hopefully today or tomorrow.

Thanks!
Fei

public class FhirRestfulServlet extends RestfulServer {
@Inject
PatientProvider patientProvider = null;

public FhirRestfulServlet() {
    super(FhirContext.forDstu2());
}

@Override
protected void initialize() throws ServletException
{
    /*
     * The servlet defines any number of resource providers, and
     * configures itself to use them by calling
     * setResourceProviders()
     */
    List<IResourceProvider> resourceProviders = new ArrayList<IResourceProvider>();
    resourceProviders.add(this.patientProvider);
    setResourceProviders(resourceProviders);
}

}

public class PatientProvider implements IResourceProvider {

@Search()
public List<Patient> getPatient(@RequiredParam(name = Patient.SP_IDENTIFIER) StringParam identifier) {

}
}

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Feb 3, 2016

Hi Fei,

Interesting problem :)

Is the issue that because of the proxy that Weld creates, the server sees

@Search()
public List getPatient(@RequiredParam(name = Patient.SP_IDENTIFIER) StringParam identifier) {

instead of

@Search()
public List<Patient> getPatient(@RequiredParam(name = Patient.SP_IDENTIFIER) StringParam identifier) {

?

I'm actually kind of surprised this would be an issue- The IResourceProvider interface has the getResourceType() method which the server uses to explicitly find out what resource type is being used. I guess that isn't working for some reason in this case though.

Is there any error message or stack trace you see? Could you paste that in here?

@fw060

This comment has been minimized.

fw060 commented Feb 3, 2016

Hi James,
Yes that is the exactly the problem! :)

The stack trace is attached. The exact issue is that the line 424 in BaseMethodBinding (with latest snapshot):
returnTypeFromMethod = ReflectionUtil.getGenericCollectionTypeOfMethodReturnType(theMethod);
would return null for the Proxy class because the line 88 in ReflectionUtil would not return ParameterizedType instance.

Thanks,
Fei

2016-02-03 15:48:57,541 ERROR [io.undertow.request] [LMMFW/ANONYMOUS:NO_HTTP_CONTEXT@default task-1] UT005023: Exception handling request to /gil/fhir/Observation/39624754: javax.servlet.ServletException: Failed to initialize FHIR Restful server
at ca.uhn.fhir.rest.server.RestfulServer.init(RestfulServer.java:747)
at javax.servlet.GenericServlet.init(GenericServlet.java:244)
.....................
at java.lang.Thread.run(Thread.java:745)
Caused by: java.lang.NullPointerException
at ca.uhn.fhir.rest.method.BaseMethodBinding.isResourceInterface(BaseMethodBinding.java:557)
at ca.uhn.fhir.rest.method.BaseMethodBinding.bindMethod(BaseMethodBinding.java:425)
at ca.uhn.fhir.rest.server.RestfulServer.findResourceMethods(RestfulServer.java:314)
at ca.uhn.fhir.rest.server.RestfulServer.findResourceMethods(RestfulServer.java:303)
at ca.uhn.fhir.rest.server.RestfulServer.registerProviders(RestfulServer.java:1025)
at ca.uhn.fhir.rest.server.RestfulServer.init(RestfulServer.java:729)
... 49 more

@fw060

This comment has been minimized.

fw060 commented Feb 4, 2016

Hi James, I just submitted the pull request. Basically I added a Map from resource type to class and use it later. This is pure optional so the existing functionality should not be impacted. Please let me know what do you think about this.

Thanks!
Fei

@jamesagnew jamesagnew closed this in 0ff111b Feb 6, 2016

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Feb 6, 2016

Hi Fei,

I had a try at creating a unit test to recreate this, and as it turns out I think the issue is really just that we have a check to prevent people returning List of the wrong type, and it was failing if there was no type.

I think that with the fix in 0ff111b we don't need the extra method from your pull request- are you able to confirm if this fixes things in your setup?

Thanks for all the analysis!

@fw060

This comment has been minimized.

fw060 commented Feb 9, 2016

Hi James,
Yes, your fix works. This was actually the stop gap fix we have been using until we come up with the proposed solution. If the intention of the check is to just to prevent wrong type rather than ensure right type (thus no type is not allowed), then your fix works perfectly. We probably should have checked with you earlier. Thanks again for your time to analyze and fix this!

Another question will this fix be in 1.4 or 1.5? It was checked in before version bump but after the 1.4 label was created. Hopefully it is in version 1.4 :)

Thanks!
Fei

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Feb 9, 2016

Hi Fei,

Yeah, I think just watering the check down is good enough. It's intended to
be a last resort thing, but nothing more. So I think we can call this one
closed :)

Unfortunately this fix didn't make it into 1.4. It's already in the most
recent 1.5 snapshot build though, and it'll be a part of the 1.5 release.

Cheers,
James

On Tue, Feb 9, 2016 at 9:46 AM, fw060 notifications@github.com wrote:

Hi James,
Yes, your fix works. This was actually the stop gap fix we have been using
until we come up with the proposed solution. If the intention of the check
is to just to prevent wrong type rather than ensure right type (thus no
type is not allowed), then your fix works perfectly. We probably should
have checked with you earlier. Thanks again for your time to analyze and
fix this!

Another question will this fix be in 1.4 or 1.5? It was checked in before
version bump but after the 1.4 label was created. Hopefully it is in
version 1.4 :)

Thanks!
Fei


Reply to this email directly or view it on GitHub
#291 (comment)
.

@fw060

This comment has been minimized.

fw060 commented Feb 9, 2016

Yes, now we can call this closed :)

What is current plan (release time) for 1.5 release?

Thanks,
Fei

@jamesagnew

This comment has been minimized.

Owner

jamesagnew commented Feb 9, 2016

We don't yet have a release date in mind for 1.5, but snapshots are available now and we have been typically following a 2-3 month release cycle. I would say that at the latest, it will be released before the May HL7 Working Group Meeting.

@fw060

This comment has been minimized.

fw060 commented Feb 10, 2016

Hi James,
May is probably too late for us. So I m exploring other options. We can support our own fork. Another option is to see if 1.4.1 is an option, I m more than willing to help if HAPI team would like to start release maintenance release.

Thanks,
Fei

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