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

PAYARA-3729 EJB HTTP Endpoint improvments #3926

Merged
merged 8 commits into from May 6, 2019

Conversation

Projects
None yet
3 participants
@jbee
Copy link
Contributor

commented May 2, 2019

While testing #3925 I looked into several kinds of errors. Some where user errors due to limitations I wasn't aware of. This PR contains the improvements I made to understand the errors I dealt with.

  • Added logging
  • Error responses are less likely to mislead as different error causes are distinguished
  • Separated concerns on method level into: unpack JSON, process request, create response + helpers
  • For the interface we now don't simply use the first interface returned by getClass().getInterfaces() but make use of the EJB bean name in case it does contain the interface name
  • For application name we do use the name given in the EJB bean name if it contains the application name (portable names) - otherwise we do fallback on the existing strategy to try each application and use the first that succeeds. This does however now only continue trying if the problem was looking up the bean. If the operation done with the bean fails this is considered the final error and no further applications are tried.
  • uses thread's context classloader to load parameter classes

Ignore the changes to the pom as those are part of #3925. This PR just branched from that branch as I did both at the same time.

Test app can be found here payara/payara-samples#8

@jbee jbee requested review from cubastanley, arjantijms and pdudits May 2, 2019

@jbee jbee self-assigned this May 2, 2019

@jbee jbee added this to the 5.192 milestone May 2, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

jenkins test please

jbee added some commits May 2, 2019

@jbee

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

jenkins test please

.getMethod(requestPayload.getString("method"), argTypes)
.invoke(bean, argValues);

Object result = invokeBeanMethod(beanName, methodName, argTypes, argValues, principal, credentials);
response.setContentType(APPLICATION_JSON);
response.getWriter().print(result instanceof String? result : JsonbBuilder.create().toJson(result));

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

Well if the protocol is like that, and Strings works in current client, then we should use text/plain in this case. Because as application/json the String needs to be quoted

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

I does work indeed but you are correct. I wonder why it was done this way originally. Maybe we can just use JSON in any case.

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

I changed this so it always sends JSON which also works.

logger.log(Level.WARNING, "EJB bean method invocation failed.", ex);
response.sendError(SC_INTERNAL_SERVER_ERROR,
"Error while invoking invoking method " + methodName + " on EJB with name " + beanName + ": "
+ ex.getMessage());

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

And error handling in client is likely next topic to look at.

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

Yes, one I know and find irritating is that the client does not detect that the endpoint isn't enabled. That said @arjantijms reminded me that for security reasons we have to be careful how much information we do expose to the user.

boolean success = excuteInAppContext(() -> {
} else {
String methodName = requestPayload.getString("method");
Class<?>[] argTypes = toClasses(requestPayload.getJsonArray("argTypes"));

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

I believe you need to use app classloader for that (but I can take care of it as part of "Full JSON-B support" issue)

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

Good catch. As this could break something I think this PR should not introduce a new problem.

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

Since it used plain Class.forName before, it will not be a new problem :)

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

It did but it did so within excuteInAppContext I think which does set the app classloader as context classloader which should address this if I am not mistaken. Therefore I moved that into the lambda and thereby into the right context again.

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

No, you I think you need to use Class.forName(className, true, contextClassLoader)

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

Somehow I was thinking the context classloader would be involved in Class.forName(String). Changed it to use the context classloader. It should be more correct this way. Tested it as well and used the change to test redeploying the invoker app using the admin console which also works (as long as one sets the correct name/context root).

protected void doPost(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
JsonObject requestPayload = readJsonObject(request.getReader());

This comment has been minimized.

Copy link
@pdudits

pdudits May 3, 2019

Contributor

Another error case would be malformed Json

This comment has been minimized.

Copy link
@jbee

jbee May 3, 2019

Author Contributor

I did not address this since the caller usually is our client which we should make sure does not use malformed JSON. While being a possible problem it does not seemed so important to handle to me.

PAYARA-3729 addressed Patrik's comments: respond as JSON, use app cla…
…ssloader to create classes and objects
@pdudits

pdudits approved these changes May 3, 2019

@pdudits

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

jenkins test please

@pdudits pdudits merged commit 1b6a934 into payara:master May 6, 2019

59 checks passed

Payara Quick Build and Test Quick build and test passed!
Details
security/snyk - api/payara-api/pom.xml (payara-ci) No new issues
Details
security/snyk - api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/admingui/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ant-tasks/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/appclient/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/batch/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/common/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/concurrent/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/connectors/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/core/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ejb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/extras/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/featuresets/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/ha/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/installer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/javaee-api/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jdbc/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/jms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/load-balancer/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/orb/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/payara-appserver-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/persistence/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/registration/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/security/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/transaction/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/web/pom.xml (payara-ci) No new issues
Details
security/snyk - appserver/webservices/pom.xml (payara-ci) No new issues
Details
security/snyk - copyright/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/admin/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/cluster/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/common/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/core/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/deployment/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/diagnostics/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/distributions/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/flashlight/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/grizzly/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/hk2/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/osgi-platforms/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/packager/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/payara-modules/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources-l10n/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/resources/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/security/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/test-utils/pom.xml (payara-ci) No new issues
Details
security/snyk - nucleus/tests/pom.xml (payara-ci) No new issues
Details
security/snyk - pom.xml (payara-ci) No new issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.