Skip to content

Extension mechanism / Groups Management APIs#620

Merged
thibauult merged 32 commits intofinos:mainfrom
thibauult:feature/extension-mechanism
Jan 14, 2022
Merged

Extension mechanism / Groups Management APIs#620
thibauult merged 32 commits intofinos:mainfrom
thibauult:feature/extension-mechanism

Conversation

@thibauult
Copy link
Copy Markdown
Member

@thibauult thibauult commented Jan 6, 2022

The BDK extension mechanism aims to allow developers to provide additional features without necessary having to contribute in the finos/symphony-bdk-java repository.

All extensions must implement the BdkExtension marker interface, and can optionally implement extension point interfaces such as:

  • BdkExtensionServiceProvider
  • BdkApiClientFactoryAware
  • BdkAuthenticationAware
  • BdkRetryBuilderAware

As a first use-case, this PR also brings the Symphony's Groups API support as an extension.


Todo list

  • 🏁 unit tests
  • ♻️ Spring Boot support
  • ✍️ javadoc
  • ✍️ readme doc

Closes #479

…echanism

# Conflicts:
#	symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/ApiClient.java
#	symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/auth/Authentication.java
#	symphony-bdk-http/symphony-bdk-http-jersey2/src/main/java/com/symphony/bdk/http/jersey2/ApiClientJersey2.java
#	symphony-bdk-http/symphony-bdk-http-webclient/src/main/java/com/symphony/bdk/http/webclient/ApiClientWebClient.java
Comment thread symphony-bdk-ext/symphony-groups-ext/build.gradle
Comment thread symphony-bdk-ext/symphony-groups-ext/build.gradle Outdated

private final ApiClient loginClient;

public String retrieveBearerToken(@Nonnull final AuthSession session) throws ApiException {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we handle the token's refresh for now?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes that's not done yes 👍 I was actually thinking of implementing the Groups API as part of a separate PR to make this one clearer and with a single purpose. WDYT?

@API(status = API.Status.EXPERIMENTAL)
public interface BdkAuthenticationAware {

void setAuthSession(AuthSession session);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

so I guess breaking the dependency on core means moving AuthSession, ApiFactory, Retry to interfaces and in the extension-api module?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Correct, I actually started to move the Retry logic to a separate module but it would bring quite a lot of additional changes that will make this PR bigger. I'd prefer to do it as part of another PR once this one merged

* TODO javadoc
*/
@API(status = API.Status.EXPERIMENTAL)
public interface BdkApiClientFactoryAware {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

something we have not thought of maybe is how do i inject my own things in the extension? given I don't manage the creation of it
We should also perhaps implement the spring boot support too to see if it fits in

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

well right now we create what we need in the SymphonyGroupService constructor (for instance the oauth client)
but what if I want an existing object?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What to you mean by existing object?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

something that the extension would need but that would not be managed by the BDK itself, let's say that the extension needs to encrypt data before sending it via the generated api classes
so it needs an Encryptor object but also want to share it across multiple extensions so we would like to inject it in different places (like the ApiFactory that we inject in the extension but for an object not created/managed by the BDK)

(yes I'm making this up and maybe this is too complex for now)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This would be possible if the extension provides a service, this service could expose an init() methods that takes external objects not necessary managed by the BDK. Something like:

MyExtensionService service = bdk.extensions().service(MyExtension.class);
service.configure(encryptionModule);
// then
String encryptedData = service.encrypt("Hello, World!");

@thibauult thibauult marked this pull request as ready for review January 10, 2022 17:59
@thibauult thibauult requested a review from a team as a code owner January 10, 2022 17:59
thibauult and others added 2 commits January 10, 2022 21:03
Not sharing it with the core as the configuration is a bit more
specific.
@@ -0,0 +1,61 @@
# Symphony Group Extension
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe just a generic extension documentation as part of docs/ is enough?

public class GroupExtensionConfig {

@Bean
public SymphonyGroupService groupService(BdkConfig config, ApiClientFactory apiClientFactory, AuthSession session) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can we do more magic and pick it up at runtime just because it is in the classpath? like spring bot does when add a dependency?

@thibauult thibauult added this to the 2.6.0 milestone Jan 11, 2022
Comment thread docs/extension.md
@thibauult thibauult changed the title Extension mechanism Extension mechanism / Groups Management APIs Jan 11, 2022
Comment thread docs/extension.md
@RequestMapping("/api")
public class ApiController {

@Lazy // required, otherwise Spring Boot application startup will fail
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

can't we put the lazy on the bean instead?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Comment thread docs/extension.md
```

### Access your Extension's service in Spring Boot
In Spring Boot, your extension's service is _lazily_ initialized. It means that you must annotate your injected extension's service
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We could do it with https://www.baeldung.com/spring-boot-custom-auto-configuration too
But that would probably forces us to have a spring boot group extension module

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeaaaah 839c08d

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🪄 🐰 🎩

@thibauult thibauult force-pushed the feature/extension-mechanism branch from 5b2344c to eea68f4 Compare January 13, 2022 19:58
Copy link
Copy Markdown
Contributor

@symphony-elias symphony-elias left a comment

Choose a reason for hiding this comment

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

LGTM

@thibauult thibauult force-pushed the feature/extension-mechanism branch from 55ad660 to b968e41 Compare January 14, 2022 11:03
…cally loaded when the extension is declared as a bean (requires `@Lazy`). However, the SymphonyGroupService can directly be added as a bean (no need to register the extension in this case)
…echanism

# Conflicts:
#	symphony-bdk-http/symphony-bdk-http-api/src/main/java/com/symphony/bdk/http/api/auth/Authentication.java
#	symphony-bdk-http/symphony-bdk-http-jersey2/src/main/java/com/symphony/bdk/http/jersey2/ApiClientJersey2.java
#	symphony-bdk-http/symphony-bdk-http-webclient/src/main/java/com/symphony/bdk/http/webclient/ApiClientWebClient.java
@thibauult thibauult merged commit e4c7ccc into finos:main Jan 14, 2022
@thibauult thibauult deleted the feature/extension-mechanism branch January 14, 2022 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Module system

4 participants