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

Refuse to use a decorating Client and Service if as() or serviceAdded() is not overriden. #1958

Closed
trustin opened this issue Aug 5, 2019 · 8 comments
Closed

Refuse to use a decorating Client and Service if as() or serviceAdded() is not overriden. #1958

trustin opened this issue Aug 5, 2019 · 8 comments
Labels
good first issue improvement

Comments

@trustin
Copy link
Member

trustin commented Aug 5, 2019

A common mistake a user makes when writing a decorator is:

ServerBuilder sb = new ServerBuilder();
sb.service("/svc", myService.decorate(delegate -> (ctx, req) -> {
    ...
    return delegate.serve(ctx, req);
}));

The above decorator is not correct because the returned service does not override the default as() and serviceAdded() implementation. A correct usage is:

sb.service("/svc", myService.decorate((delegate, ctx, req) -> {
    return delegate.serve(ctx, req);
});
// or
sb.service("/svc", myService.decorate(delegate -> new SimpleDecoratingService(delegate) {
    ...
    return delegate().serve(ctx, req);
    ...
}));

We can detect such a mistake by checking if the decorator overrides the default as() and serviceAdded() implementation using Java reflection API and raise an exception.
@thinkgruen
Copy link
Contributor

I would like to also tackle this one :)

@trustin
Copy link
Member Author

trustin commented Aug 11, 2019

Thank you! 🙇‍♂️

@thinkgruen
Copy link
Contributor

Sorry guys, a month has already passed and I yet couldn't get to it. I will try to work on it over the course of the next week. 😞

@trustin
Copy link
Member Author

trustin commented Sep 6, 2019

No worries. I really appreciate that you're trying to help us! 🙇‍♂️

@jacobis
Copy link

jacobis commented Feb 18, 2020

@trustin Could I try this issue?

@ikhoon
Copy link
Contributor

ikhoon commented Feb 18, 2020

I'm not trustin, but I could tell "Yes, please!" 😄

@trustin
Copy link
Member Author

trustin commented Feb 18, 2020

🤣 Yes, please!

@trustin trustin added this to the 0.99.0 milestone Mar 17, 2020
ikhoon added a commit to ikhoon/armeria that referenced this issue Mar 18, 2020
@ikhoon
Make Client and Service decorator always override as() or `serv…
c013b9d 
…iceAdded()`

Motivation:

If a user writes a decorator with curried lambda style,
the given decorator will not override `as()` or `serviceAdded()`. line#1958

```java
sb.service("/svc", myService.decorate(delegate -> (ctx, req) -> {
    ...
    return delegate.serve(ctx, req);
}));
```
The decorated will not call `ServiceCallbackInvoker.invokeServiceAdded()` properly.

Modifications:

- Add `DecoratorUtil` for validating decorators using reflection.
- Add the validation logic to `{Http,Rpc}Service.decorate()`, `ClientDecoration.*decorate()`, `RouteDecoratingService()`.

Result:

You can now get `IllegalArgumentException` when the specified decorator does not override `as()` or `serviceAdded()`
ikhoon added a commit to ikhoon/armeria that referenced this issue Mar 18, 2020
@ikhoon
Make Client and Service decorator always override as() or `serv…
c58b635 
…iceAdded()`

Motivation:

If a user writes a decorator with curried lambda style,
the given decorator will not override `as()` or `serviceAdded()`. line#1958

```java
sb.service("/svc", myService.decorate(delegate -> (ctx, req) -> {
    ...
    return delegate.serve(ctx, req);
}));
```
The decorated will not call `ServiceCallbackInvoker.invokeServiceAdded()` properly.

Modifications:

- Add `DecoratorUtil` for validating decorators using reflection.
- Add the validation logic to `{Http,Rpc}Service.decorate()`, `ClientDecoration.*decorate()`, `RouteDecoratingService()`.

Result:

You can now get `IllegalArgumentException` when the specified decorator does not override `as()` or `serviceAdded()`
ikhoon added a commit to ikhoon/armeria that referenced this issue Mar 18, 2020
@ikhoon
Make Client and Service decorator always override as() or `serv…
6f2e0f7 
…iceAdded()`

Motivation:

If a user writes a decorator with curried lambda style,
the given decorator will not override `as()` or `serviceAdded()`. line#1958

```java
sb.service("/svc", myService.decorate(delegate -> (ctx, req) -> {
    ...
    return delegate.serve(ctx, req);
}));
```
The decorated will not call `ServiceCallbackInvoker.invokeServiceAdded()` properly.

Modifications:

- Add `DecoratorUtil` for validating decorators using reflection.
- Add the validation logic to `{Http,Rpc}Service.decorate()`, `ClientDecoration.*decorate()`, `RouteDecoratingService()`.

Result:

You can now get `IllegalArgumentException` when the specified decorator does not override `as()` or `serviceAdded()`
Fixes line#1958
@ikhoon ikhoon mentioned this issue Mar 18, 2020
Closed
@trustin trustin removed this from the 0.99.0 milestone Mar 19, 2020
@trustin
Copy link
Member Author

trustin commented Mar 19, 2020

I've just realized this stops a user from using Function.identity() or other creative use of functions, like returning a completely different Client or Service which is not a decorator.

That means, I think we can't validate anything this way. 😭

@trustin trustin closed this as completed Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
good first issue improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Make Client and Service decorator always override as() or serviceAdded() ikhoon/armeria
4 participants
@trustin @jacobis @ikhoon @thinkgruen