-
Notifications
You must be signed in to change notification settings - Fork 42
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
[WEJBHTTP-142] First round of refactorings in EJB part - focus on client side of the protocol #219
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some feedback
ejb/src/main/java/org/wildfly/httpclient/ejb/RequestBuilder.java
Outdated
Show resolved
Hide resolved
ejb/src/main/java/org/wildfly/httpclient/ejb/RequestBuilder.java
Outdated
Show resolved
Hide resolved
import org.wildfly.httpclient.common.Protocol; | ||
|
||
import java.lang.reflect.Method; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of class and its purpose? Sample usage example would be nice as well, as certain methods need to be invoked before others (e.g. setting the request type).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a typo in the header of a commit, usealess
ejb/src/main/java/org/wildfly/httpclient/ejb/HttpEJBInvocationBuilder.java
Outdated
Show resolved
Hide resolved
ejb/src/main/java/org/wildfly/httpclient/ejb/HttpEJBInvocationBuilder.java
Outdated
Show resolved
Hide resolved
… distinctName & beanName method parameters. These values are available as instance fields.
…request method, second set HTTP request path and finally set HTTP request headers.
…ry to call UTF-8.name() method which may throw UnsupportedEncodingException.
…ring() conversion method. StringBuilder supports appending of booleans directly.
…ds parameter. We can distinguish proper HTTP request prefix path via HttpEJBInvocationBuilder method signatures.
…troducing appendPath(StringBuilder,String) utility method.
…emoving useless 'public' method modifier.
… getters. Making all its method arguments final.
…dPath() into appendPath() & appendEncodedPath() methods. Adjusting code accordingly.
…anPath() & buildModulePath() into buildBeanPath() method.
…, distinctName & beanName method parameters. These are available as instance fields.
…ath() and appendEncoedPath() utility methods.
…ath() & appendedEncodedPath() methods.
…d, invocationId & cancelIfRunning method parameters. These are available as instance fields.
…BeanRequestPath() method.
…BeanRequestMethod() method.
…eanPath() method to appendBeanPath() and changing its parameters order.
…RequestHeaders() method.
…eanRequestMethod() to setRequestMethod().
…eanRequestPath() to setRequestPath().
…cation type is not handled properly.
…variable in HttpEJBReceiver.
…tionType enum values.
…eName, distinctName & beanName strings in HttpEJBInvocationBuilder.
…ppendOperationPath() method.
…uilder's appendOperationPath() method call one method up in the caller's stack.
…JBInvocationBuilder class for configuring client requests.
…s parameter 'mountPoint' to 'prefix'.
…nRequestPath() methods.
…e' field to 'requestType'.
…r in RequestType enum.
…in RequestType enum.
…over default one.
…rom HttpEJBReceiver to RequestBuilder.
8ef967e
to
b70a8f8
Compare
Typo 'usealess' was fixed in PR refresh Flavia. |
@fl4via @rachmatowicz please review again. |
b70a8f8
to
c522932
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am approving the PR and merging it with one thing to consider as future improvement: the naming of OPEN request does not sound correct to me and it should be renamed.
@@ -283,7 +283,7 @@ protected SessionID createSession(final EJBReceiverSessionCreationContext receiv | |||
CompletableFuture<SessionID> result = new CompletableFuture<>(); | |||
|
|||
RequestBuilder builder = new RequestBuilder() | |||
.setRequestType(RequestType.CREATE_SESSION) | |||
.setRequestType(RequestType.OPEN) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because there is no open operation in the EJB spec, I am not very fond of this renaming. CREATE_SESSION implies that we are creating the session, which is what we actually are doing here. OPEN is confusing. We use open to refer to transactions, resources, connections, but to me it does not relate to the create session operation.
https://issues.redhat.com/browse/WEJBHTTP-142