Add Delegator to all API Implementations

[adnanhemani]

Draft PR of the discussion on the Dev ML regarding using the Delegator pattern to add Events instrumentation to all Polaris APIs.

[dimas-b]

I'm against the term "business logic". I find it very confusing. How about PublicApi?

[adnanhemani]

I'm not sure @PublicApi captures what we're trying to say here. What do you think about something like @ApiBaseImplementation?

[dimas-b]

I see your idea now, I think 😅

However, I'm not sure this is the best approach. Current delegators tae concrete classes into their constructors - this reduces option for composing the delegators with other implementations.

Since delegation is apparently covering API interfaces, I believe it is best to have one delegator per *ApiService interface.

Then, I propose to use @MainService as a @Qualifier for PolarisServiceImpl, etc.

For completeness, we should probably use a qualifier like @EventsService for delegators that introduce events.

This way, both sets of services can be disambiguated in this repo and in downstream projects.

[dimas-b]

What do we achieve by always using these delegating wrappers?

[adnanhemani]

Once we get these delegating wrappers in, we can start adding the events instrumentation to these wrappers (as discussed on the Dev ML, based on the feedback from the community).

For example, the code in #1904 would then be re-written to go into these delegator classes instead.

[dimas-b]

Should we have something like PolarisServiceWithEvents?.. or EventProducingPolarisService?

The impl. will follow the delegator pattern. It's the "Default Delegator" in the class name that make me confused.

[adnanhemani]

Thanks for your feedback, @dimas-b! I've updated the PR accordingly - please let me know if there is further feedback :)

[adutra]

Suggested change

	// Used to annotate delegator classes, which are emitting events
	/** Used to annotate delegator classes, which are emitting events. */

[adnanhemani]

Changed!

[adutra]

I like the general direction we are taking in this PR, but wanted to mention that this CDI pattern has dedicated annotations: @Decorator and @Delegate.

It's explained here:

• https://quarkus.io/guides/cdi#decorators
• https://jakarta.ee/specifications/cdi/3.0/jakarta-cdi-spec-3.0#decorator_bean

However, these annotations require both decorator and delegate beans to implement one common interface. This might not be possible for beans like IcebergCatalogAdapter since it's not an interface. We'd need to extract an interface from it in order to make the Decorator CDI pattern work.

Here, it could look like this:

@RequestScoped
@Default
@EventsServiceDelegator
@Decorator
@Priority(1000) // Will allow downstream project-specific delegators to be added and used
public class PolarisCatalogsServiceDefaultDelegator implements PolarisCatalogsApiService {

  private final PolarisCatalogsApiService delegate;

  @Inject
  public PolarisCatalogsServiceDefaultDelegator(@Delegate @MainService PolarisCatalogsApiService delegate) {
    this.delegate = delegate;
  }

[adutra]

That said, I see that most adapters generally implement one or more interfaces, e.g.

@RequestScoped
@MainService
public class IcebergCatalogAdapter
    implements IcebergRestCatalogApiService, IcebergRestConfigurationApiService, CatalogAdapter {

So, maybe using @Decorator could work out of the box, if both beans (decorator and delegate) implement exactly the same interfaces (and there is no ambiguity when resolving beans).

[adnanhemani]

This is a good suggestion, it helps make the code a bit tighter as well. Added in the next revision!

[adutra]

nit: do we need these dummy javadocs?

[adnanhemani]

These are copied from the original file, I didn't remove them. Happy to remove if you're more comfortable without them, but I originally didn't see any reason to remove these.

[adutra]

Hm what original file? I thought this file was new. This is a minor point though, so feel free to handle it as you see fit.

[adnanhemani]

Sorry, I meant it came from PolarisServiceImpl.java, which was the base file that I was using for creating this file. Let me remove.

[adutra]

nit: the naming convention is a bit odd, I'd expect the decorator to be named <DelegateName>EventServiceDelegator or something like that.

[adnanhemani]

hmm - I've tried naming everything into <DelegateName>DefaultDelegator rather than "...EventServiceDelegator" because in the following PRs, we will add the Events instrumentation to this Delegator. In other words, generating Events will be the default delegator. WDYT?

[adutra]

So... what happens if we need delegators for something else? E.g. PolarisDiagnostics could be turned into another delegator, e.g. <DelegateName>Diagnostics.

The beauty of @Decorator is that you can in theory have as many decorators you need, and each time the decorated interface is injected, CDI would transparently create a chain of invocations that traverse all the decorators up to the "undecorated" bean. IOW if you inject PolarisCatalogsApiService somewhere, calling PolarisCatalogsApiService.createCatalog() would actually call <DelegateName>Diagnostics.createCatalog() then <DelegateName>EventServiceDelegator.createCatalog() then PolarisServiceImpl.createCatalog().

[adnanhemani]

I understand what you are saying now - I think I was originally closer to Dmitri's idea earlier in this PR by attempting to use a delegator wrapper without the Decorator pattern. Let me put a general comment on the PR regarding this and change the naming convention to more accurately reflect the Decorator pattern.

[adutra]

This may not be necessary anymore when using proper interfaces and @Decorator / @Delegate.

[adnanhemani]

You're correct - removed!

[adutra]

Thanks @adnanhemani for this proposal that seems to go into a clean direction.

[adutra]

That said, I see that most adapters generally implement one or more interfaces, e.g.

@RequestScoped
@MainService
public class IcebergCatalogAdapter
    implements IcebergRestCatalogApiService, IcebergRestConfigurationApiService, CatalogAdapter {

So, maybe using @Decorator could work out of the box, if both beans (decorator and delegate) implement exactly the same interfaces (and there is no ambiguity when resolving beans).

[adnanhemani]

Thanks @adutra for your feedback, I've push the revision addressing all the comments!

[adnanhemani]

Changed!

[adnanhemani]

These are copied from the original file, I didn't remove them. Happy to remove if you're more comfortable without them, but I originally didn't see any reason to remove these.

[adnanhemani]

hmm - I've tried naming everything into <DelegateName>DefaultDelegator rather than "...EventServiceDelegator" because in the following PRs, we will add the Events instrumentation to this Delegator. In other words, generating Events will be the default delegator. WDYT?

[adnanhemani]

This is a good suggestion, it helps make the code a bit tighter as well. Added in the next revision!

[adnanhemani]

You're correct - removed!

[adutra]

LGTM thanks @adnanhemani – the naming convention is still imo a bit strange but we can change that later.

[dimas-b]

LGTM overall 👍

[dimas-b]

maybe nitpicky, but "delegator" feels odd to me in a qualifier. A qualifier is supposed highlight a functional aspect of a bean, IMHO. Whether or not the bean delegates to other beans or genuinely implements and API should be be exposed in a qualifier, IMHO.

Would ServiceWithEvents or EventsEmitter (I assume it's supposed to emit events eventually) work?

[dimas-b]

I'm fine addressing naming concerns later if there's a general understanding that class and qualifier names will change :)

[dimas-b]

Using decorators is an interesting approach.

However, AFAIK, decorators are impossible to remove in downstream builds, right? I mean that if a decorator is discovered, it will be applied and all decorators will be applied in priority order. This means that all downstream users or runtime/service will always get this decorator in call paths.

This is fine, but I believe it might be wise to isolate the decorator in a new module (jar) to allow downstream projects to opt out of it.

[adutra]

That is correct. This situation is generally solved by either deactivating the decorator through some configuration flag, or, as you pointed out, by isolating the decorators in separated modules that integrators are free to include or exclude.

[dimas-b]

Good point. I suppose @IfBuildProperty(...) might be good enough for now (without introducing new modules).

... or perhaps @UnlessBuildProperty(...).

[adnanhemani]

Agreed here with both of you. I'm leaning towards the @UnlessBuildProperty - but am thinking to leave this PR without it and let a future contribution make that change if anyone requires it.

[adutra]

That works for me 👍

[dimas-b]

Fair enough

[dimas-b]

The idea in my previous comment when I suggested using custom qualifiers was to use them for composing the delegation chain of *ApiService implementations.

However, if we go with the decorator approach, I believe this qualifier becomes rather useless 🤔 WDYT?

[adutra]

I asked myself the same question - but it all depends on how we are going to use these qualifiers, so I figured we would figure that out later 😅

[adnanhemani]

I think you're right @dimas-b. Let me remove it for now and then if we think it'll be useful later, we can always reintroduce it in a new change!

[adnanhemani]

Thanks all for the feedback! There was a gradual shift from originally using a delegator wrapper to using the Decorator pattern in this PR - but I think we have reached convergence on this pattern based on the comments in this PR.

Naming conventions are still not finalized, but I believe this is the closest we have been so far to something usable as a base. I will be releasing further PRs using this as a base shortly for further event instrumentation.

[adnanhemani]

I understand what you are saying now - I think I was originally closer to Dmitri's idea earlier in this PR by attempting to use a delegator wrapper without the Decorator pattern. Let me put a general comment on the PR regarding this and change the naming convention to more accurately reflect the Decorator pattern.

[adnanhemani]

Agreed here with both of you. I'm leaning towards the @UnlessBuildProperty - but am thinking to leave this PR without it and let a future contribution make that change if anyone requires it.

[adnanhemani]

Sorry, I meant it came from PolarisServiceImpl.java, which was the base file that I was using for creating this file. Let me remove.

[adnanhemani]

I think you're right @dimas-b. Let me remove it for now and then if we think it'll be useful later, we can always reintroduce it in a new change!

[adutra]

Hmm I think @Decorator is mutually exclusive with @Alternative.

Alternative beans provide a whole different bean implementation. Use it when you want to suppress the original bean's behavior. It's similar to mocking.

Decorators on the other hand are used to further specialize the original bean's behavior, without suppressing it. It's similar to AOP interceptors.

IMO what we are trying to achieve here is the second approach where the original bean keeps doing its job, but we add behavior before/after each invocation of the bean.

[adnanhemani]

You are correct - I misread some documentation while looking up what would be required if we wanted to use @UnlessBuildProperty in the future and thought that @Alternative would be required at that time. Let me remove that property and push again!

[RussellSpitzer]

Looks good to me, I learned a lot about the delegator annotations allowed in the CDI from the discussion and it seems like this is a very straightforward pattern.

[dimas-b]

nit: It might be worth adding a public constant for this priority value so that downstream projects could reference it... but it can be done later.