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.

Sign up for GitHub

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

Jump to bottom

APP-3032: Revamping of the API Invoker #234

Merged
merged 10 commits into from
Sep 24, 2020
Merged

APP-3032: Revamping of the API Invoker #234

merged 10 commits into from
Sep 24, 2020

Conversation

symphony-hong
Copy link
Contributor

@symphony-hong symphony-hong commented Sep 21, 2020 •
edited
Loading

Ticket

3032

Description

BDK: Revamping of the API Invoker

Dependencies

List the other pull requests that should be merged before/along this one.

Checklist

  • Referenced a ticket in the PR title and in the corresponding section
  • Filled properly the description and dependencies, if any
  • Unit tests updated or added
  • Javadoc added or updated
  • Updated the documentation in docs folder

@symphony-hong
Rename symphony-bdk-core-invokers module
ce7d0ee 
symphony-bdk-core-invokers -> symphony-bdk-http
symphony-bdk-core-invoker-api -> symphony-bdk-http-api
symphony-bdk-core-invoker-jersey2 -> symphony-bdk-http-jersey2

HttpClient to fluently call ApiClient when used outside generated code.

Unittest added.

**Documentation not ready yet.**
@symphony-hong
Merge upstream/master
fd1b55f
@symphony-hong
Merge upstream/master
ffe841c
@symphony-hong symphony-hong changed the title App 3032 APP-3032: Revamping of the API Invoker Sep 21, 2020
symphony-youri
symphony-youri reviewed Sep 22, 2020
View reviewed changes
Copy link
Contributor

@symphony-youri symphony-youri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the goal of the http modules is to be able to choose the http client implementation you want to use, do we have reasons to offer that possibility vs just choosing one and sticking to it?

symphony-bdk-core/build.gradle Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated
import javax.ws.rs.core.GenericType;

@Slf4j
public class HttpClient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our http client looks very similar to the jersey one, will this api work with other clients? (we could try openfeign and the jdk ones for instance)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Spring WebClient (priority for SpringBoot integration)

thibauult
thibauult reviewed Sep 22, 2020
View reviewed changes
symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated
import javax.ws.rs.core.GenericType;

@Slf4j
public class HttpClient {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And Spring WebClient (priority for SpringBoot integration)

symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/util/ProviderLoader.java Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/util/ProviderLoader.java Outdated Show resolved Hide resolved
symphony-bdk-core/src/main/java/com/symphony/bdk/core/HttpClient.java Outdated Show resolved Hide resolved
symphony-bdk-examples/bdk-core-examples/build.gradle Outdated Show resolved Hide resolved
@symphony-hong
Update HttpClient
1162f80 
Move the HttpClient class to symphony-bdk-http module

Create GenericClass to break the dependency of the generic http client on the jersey Generic type

Update api.mustache to use GenericClass

Update ProviderLoader to make it generic
@symphony-hong
Update SymphonyBdk entry for http client builder
941bfa9 
Javadoc updated
symphony-youri
symphony-youri approved these changes Sep 23, 2020
View reviewed changes
Copy link
Contributor

@symphony-youri symphony-youri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should be able to remove the dependency on jersey in the http api module

added a few small comments, otherwise it looks good to me, we can merge that one and then move one with the webclient in a separate PR if you prefer

symphony-bdk-core/src/main/java/com/symphony/bdk/core/util/ServiceLookup.java Outdated Show resolved Hide resolved
...xamples/bdk-core-examples/src/main/java/com/symphony/bdk/examples/HttpClientExampleMain.java
final AuthenticateRequest req = new AuthenticateRequest();
req.setToken(jwt);

Token token = bdk.http()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the http client is meant to be used for other usecases (outside of the SDK), perhaps we don't need to make it accessible in the BDK object (you would just create the Http client using the builder on its own)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this idea is suggested by @symphony-thibault to make the bdk usage more fluent. @symphony-thibault WDYT ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way developers don't need to pass the ApiClientBuilderProvider, it uses the one defined in SymphonyBdk found from ServiceLoader

symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/HttpClient.java
* HttpClient is the main entry point to fluent API used to build an {@link ApiClient} and execute
* client requests in order to consume responses returned
*/
public class HttpClient {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we want to enforce the use of the builder we can add a private constructor here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added the constructor for unittest purpose. Jacoco complained that the code coverage on HttpClient didn't reach 90% even when intellij said 100%, I think it's because the default constructor is not tested (not really sure about it)

@symphony-hong symphony-hong marked this pull request as ready for review September 23, 2020 12:12
@symphony-hong symphony-hong requested a review from a team September 23, 2020 12:12
@symphony-hong symphony-hong reopened this Sep 23, 2020
@symphony-hong symphony-hong reopened this Sep 23, 2020
@symphony-hong symphony-hong reopened this Sep 24, 2020
thibauult
thibauult approved these changes Sep 24, 2020
View reviewed changes
Copy link
Member

@thibauult thibauult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@symphony-hong symphony-hong merged commit bed6996 into finos:master Sep 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@symphony-youri symphony-youri symphony-youri approved these changes

@thibauult thibauult thibauult approved these changes

@symphony-elias symphony-elias Awaiting requested review from symphony-elias

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@symphony-hong @thibauult @symphony-youri @symphony-elias