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

feat(context): add support to watch context bindings #2122

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@raymondfeng
Copy link
Member

raymondfeng commented Dec 4, 2018

Recreated from #2111 due to branch renaming.

  • ContextView - watches matching bindings of a context and maintain
    a live collection of values
  • @inject.view - injects an array of values resolved from bindings
    that match the filter function
  • Enhance @inject and @inject.getter to accept a binding filter function

See docs.

Checklist

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

@raymondfeng raymondfeng requested a review from bajtos as a code owner Dec 4, 2018

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from bfeb532 to 36c89b6 Dec 7, 2018

Show resolved Hide resolved docs/site/Context.md Outdated
Show resolved Hide resolved docs/site/Context.md Outdated
@bajtos
Copy link
Member

bajtos left a comment

@raymondfeng I like the idea of allowing other parts of the application to subscribe to context events (a binding was registered, unregistered, etc.).

However, the current implementation seems overly complicated and complex to me. Can you try to simplify it please and using the primary use cases/scenarios to drive the requirements?

What are actually the high-level end-to-end scenarios you are trying to support? They are not clear to me. The current acceptance tests are at context level, but I'd like to see acceptance tests at application and extension (Componet) level.

Show resolved Hide resolved docs/site/Context.md Outdated
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 10, 2018

I also don't understand the connection between watching for bindings and receiving a list of values matching a filter (@inject.filter), these two concepts are orthogonal in my mind. What am I missing? Should we perhaps rename @inject.filter to @inject.watcher?

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 10, 2018

I also don't understand the connection between watching for bindings and receiving a list of values matching a filter (@inject.filter), these two concepts are orthogonal in my mind. What am I missing? Should we perhaps rename @inject.filter to @inject.watcher?

@inject.filter has two flavors:

  • inject a snapshot of values from matching bindings without watching the context. This is the behavior if the target type is an array instead of Getter or ContextWatcher.
  • inject a Getter/ContextWatcher so that it keeps track of context binding changes.
@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 10, 2018

@bajtos I extracted the ContextListener interface from ContextWatcher class to make it more flexible to create your own listeners to react on context binding events.

@raymondfeng raymondfeng force-pushed the context-watcher branch from f938235 to b03de5d Dec 10, 2018

@raymondfeng raymondfeng force-pushed the context-watcher branch 3 times, most recently from 90af513 to 145bc85 Dec 10, 2018

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 11, 2018

The key benefit of ContextWatcher is that it caches resolved values until bindings matching the filter function are added/removed. For most cases, we don't have to pay the penalty to find/resolve per request.

I am still confused. Why would we have to pay the penalty to find/resolve per request?

Here is my understanding:

  • We need to keep a Trie-based routing table populated with all known routes
  • When a new route is added (e.g. a new controller is registered) or an existing route is removed, we need to update the routing table accordingly.
  • The update can happen lazily (on the first incoming request arriving after routes were changed) or in a batch mode (processing multiple additions/removals in one go).

The new tests added by commit 145bc85 are kind of showing how to implement this, but the solution still seems unnecessary complicated to me.

What other use cases (besides updating routing table) do you have in mind for ContextWatcher implementation as you proposed here? If the only motivation is to notify the routing table about controllers being added/removed, then I believe we can find a much simpler solution.

What are actually the high-level end-to-end scenarios you are trying to support? They are not clear to me. The current acceptance tests are at context level, but I'd like to see acceptance tests at application and extension (Componet) level.

Scenario-driven development FTW!

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 12, 2018

What other use cases (besides updating routing table) do you have in mind for ContextWatcher implementation as you proposed here? If the only motivation is to notify the routing table about controllers being added/removed, then I believe we can find a much simpler solution.

The usage is much wider than just updating routing table. Basically it's the bridge between extension points and extensions using Context as the registry. Here are a few use cases:

  1. The Authentication extension point => a list of authentication strategies as extensions
  2. The RequestBodyParser extension point => a list of body parsers as extensions
  3. The RoutingTable extension point => a list of routes as extensions
  4. The LifeCycle extension point => a list of life cycle observers as extensions

Please note we use Context to register extensions for a given extension point by binding tags.

Before this PR, the extension point will receive an injection of extensions (snapshot) when it's first resolved within a context chain. The list of extensions cannot be updated afterward unless the extension point finds/resolves matching extensions per request by code.

To allow extensions for a given extension point to be added/removed after the extension point is initialized, ContextWatcher or @inject.filter can be now used to get the latest list of extensions consistently and minimize the overhead by cached values that can be invalidated/refreshed upon Context bind/unbind events.

The acceptance tests use the server as the extension point for list of controllers as extensions to illustrate the idea.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 13, 2018

Before this PR, the extension point will receive an injection of extensions (snapshot) when it's first resolved within a context chain. The list of extensions cannot be updated afterward unless the extension point finds/resolves matching extensions per request by code.

Makes sense 👍

To allow extensions for a given extension point to be added/removed after the extension point is initialized, ContextWatcher or @inject.filter can be now used to get the latest list of extensions consistently and minimize the overhead by cached values that can be invalidated/refreshed upon Context bind/unbind events.

In my mind, code (e.g. Routing table) consuming registered extensions (e.g. Controllers) needs not only an API for getting the up-to-date list of extensions (Controllers), but also a way how to be notified when that list changed.

I see the second part (subscribe to change notifications) as more important, because I am not aware of any other way how to detect changes in context bindings.

On the other hand, receiving an up-to-date list of binding matching a filter can be already implemented today by receiving the full Context object via dependency injection and using APIs like ctx.find.

minimize the overhead by cached values that can be invalidated/refreshed upon Context bind/unbind events.

This seems like a preliminary optimization to me. In the scenarios you have described above, I expect the list of bindings (extensions) to change very infrequently and therefore I expect that a simpler solution based on calling ctx.find should be fast enough for most applications.

I am not entirely opposed to implementing a caching variant, but I would like the cache to be implemented as an optional add-on built on top of a simpler abstraction.

What is the minimal API needed by places consuming a changing list of bound extension values? I am thinking about the following:

export type ChangeObserver = (
  event: ContextEventType,
  binding: Readonly<Binding<unknown>>,
) => ValueOrPromise<void>;

export type BindingMatcher = (binding: Readonly<Binding<unknown>>) => boolean;

class LiveBindings {
  // subscribe for changes
  watch(onChange: ChangeObserver, matcher?: BindingMatcher);

  // unsubscribe
  unwatch(onChange: ChangeObserver);

  // find bindings matching a pattern
  find(matcher?: BindingMatcher): Readonly<Binding<T>>[];

  // resolve given binding keys
  resolve<T>(keys: BindingAddress<T>[], session?: ResolutionSession): Promise<T[]>;
}

Maybe we don't need any new class like LiveBindings and just need to add watch/unwatch API to Context? Methods like find and resolve are already provided by Context.

A mock-up showing a possible usage in RoutingTable:

class RoutingTable {
  constructor(
    @inject(...)
    private bindings: LiveBindings
  ) {
    this.bindings.watch((event, binding) => this.onBindingsChanged(event, binding));

    for (const b of this.bindings.find()) {
      this.onBindingsChanged(ContextEventType.bind, b);
    }
  }

  onBindingsChanged(event: ContextEventType, binding: Readonly<Binding<unknown>>) {
    if (isControllerBinding(binding)) {
      switch(event) {
        case ContextEventType.bind:
          this.addControllerFromBinding(binding);
          break;
        case ContextEventType.unbind:
          this.removeControllerFromBinding(binding);
          break;
        default:
         console.warn('Ignoring unknown ContextEventType %j', event);
      }
     } else if(isRouteBinding(binding)) {
       // etc.
      }
    }
  }
}

Let's discuss.

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 13, 2018

As I am re-reading my example above, it occurs to me that the RoutingTable mock-up is too involved, which I am taking as a sign that my proposal was too low-level.

Next iteration:

type BindingChangeListener<T> = (b: Readonly<Binding<T>>) => ValueOrPromise<T>;

interface WatcherHandle {
  unsubscribe(): void;
}

interface LiveBindingWatcher {
  onAdded(b: Readonly<Binding<T>>): ValueOrPromise<T>;
  onRemoved(b: Readonly<Binding<T>>): ValueOrPromise<T>;
}

interface LiveBindings<T> {
  // subscribe for changes
  watch(listener: LiveBindingWatcher): WatcherHandle;

  // list all bindings
  find(): Readonly<Binding<T>>[];

  // resolve given binding keys
  resolve<T>(keys: BindingAddress<T>[], session?: ResolutionSession): Promise<T[]>;
}

class RoutingTable {
  constructor(
    @inject.liveList(b => b.tag === 'controller')
    private controllerBindings: LiveBindings,
    @inject.liveList(b => b.tag === 'route')
    private routeBindings: LiveBindings,
  ) {
    this.controllerBindings.watch({
      onAdded: b => this.addControllerBinding(b),
      onRemoved: b => this.removeControllerBinding(b),
    });
    for (const b of this.controllerBindings.find()) {
      this.addControllerFromBinding(b);
    }

    // process routeBindings similarly
  }
}
@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 13, 2018

@strongloop/loopback-maintainers @strongloop/loopback-next We are discussing different approaches for enhancing Context API to allow consumers to monitor changes (a new binding added, an existing binding removed). It would be great to hear about more different perspectives from you, please join the discussion if a reactive context is something you are interested in.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 13, 2018

In my mind, code (e.g. Routing table) consuming registered extensions (e.g. Controllers) needs not only an API for getting the up-to-date list of extensions (Controllers), but also a way how to be notified when that list changed.

This is exactly what I have in the PR:

  1. Enable Context to emit bind/unbind events and allow registration and notification of ContextListener.

  2. Introduce ContextWatcher as an implementation of ContextListener to maintain a getter to latest bound values matching a filter function.

On the other hand, receiving an up-to-date list of binding matching a filter can be already implemented today by receiving the full Context object via dependency injection and using APIs like ctx.find.

The ContextWatcher is introduced to not only receive up-to-date list of bound values, but also react to bind/unbind events to refresh itself.

This seems like a preliminary optimization to me. In the scenarios you have described above, I expect the list of bindings (extensions) to change very infrequently and therefore I expect that a simpler solution based on calling ctx.find should be fast enough for most applications.

Since the list of extensions is only changed infrequently, it makes more sense to cache the resolved values so that we don't have to find matching bindings every time and resolve them again. Please note that ctx.find() only give you a list of bindings.

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 13, 2018

@bajtos Your proposal is very similar to what I have:

  1. WatcherHandle: I make it part of ContextWatcher.unwatch(). We can make it consistent as part of #1972.

  2. LiveBindingWatcher ~= ContextListener

  3. LiveBindings ~= ContextWatcher

  4. @inject.liveList ~= @inject.filter

@raymondfeng raymondfeng force-pushed the context-watcher branch from 145bc85 to 58f625f Dec 13, 2018

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Dec 13, 2018

@raymondfeng raymondfeng force-pushed the context-watcher branch from 58f625f to 59a3448 Dec 13, 2018

@bajtos

This comment has been minimized.

Copy link
Member

bajtos commented Dec 14, 2018

Your proposal is very similar to what I have:

  • WatcherHandle: I make it part of ContextWatcher.unwatch(). We can make it consistent as part of #1972.

  • LiveBindingWatcher ~= ContextListener

  • LiveBindings ~= ContextWatcher

  • @inject.liveList ~= @inject.filter

Thanks, I am starting to better understand your proposed design. As usually, the pull request would have been easier to review if it was split into smaller increments, e.g. 1) ContextListener 2) ContextWatcher 3) decorator.

Anyhow.

WatcherHandle vs ContextWatcher.unwatch(): I am fine with either approach 👍

LiveBindingWatcher vs ContextListener: I find the name "ContextListener" suboptimal - we are not listening to context per se, but to events emitted by the context. How about using ContextObserver instead? Would it be compatible with the concept of observers as we understand & define it in other places (e.g. in other pull request). If not, then I think ContextEventsListener would be a better name than ContextListener. Or maybe use ContextWatcher and find a better name for the other class.

ContextWatcher: I find this name rather misleading. As I understand this class, its primary purpose is to provide an always up-to-date list of bindings and/or resolved values. The fact that we are observing/watching context is just an implementation detail. In my experience, naming code entities after implementation details leads to code and APIs that are difficult to understand. Usually it's much better to use a name describing the intent and purpose. That's how I came to LiveBindings, but that name is not perfect either. Let's try to come up with a better one.

@inject.filter: I see two issues here.

  1. I find the name very misleading - we are not injecting any filter, but a context-watcher/live-bindings instance.
  2. I think we need two different decorators, one for each use case we identified:
    1. For classes interested in push notifications (ContextListener), we need to inject something that will allow these classes to register the listener + obtain the initial list of bindings.
    2. For classes interested in pulling an always-up-to-date list of bindings and/or values, we need to inject a ContextWatcher/LiveBindings class.

@raymondfeng raymondfeng force-pushed the context-watcher branch 7 times, most recently from 0ca5b8c to e7668e4 Jan 31, 2019

@raymondfeng raymondfeng force-pushed the context-watcher branch 7 times, most recently from c7a2e0c to 355f4f6 Feb 7, 2019

@@ -229,3 +229,266 @@ class HelloController {
These "sugar" decorators allow you to quickly build up your application without
having to code up all the additional logic by simply giving LoopBack hints (in
the form of metadata) to your intent.

## Context events

This comment has been minimized.

@jannyHou

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

The PR has been rebased to master now. Please check again.

}

ctx.bind('store').toClass(Store);
expect(() => ctx.getSync<Store>('store')).to.throw(

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

Nitpick: we can test in an async wayawait expect(ctx.get<Store>('store')).to.be.rejectedWith('The type of Store.constructor[0] (String) is not a Getter function').

const view = inst.view;
expect(await view.values()).to.eql([3, 5]);
// Add a new binding that matches the filter
// Add a new binding that matches the filter

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

dup

}
});

describe('ContextEventListener', () => {

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

I feel this test suit repeated in PR #2291

});

it('resolves bindings', async () => {
expect(await contextView.resolve()).to.eql(['BAR', 'FOO']);

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

what is the difference between function resolve() and values()?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

resolve() is used for @inject.* to resolve sync/async values. values() honors the cache.

expect(contextView.bindings).to.containEql(xyzBinding);
// `abc` does not have the matching tag
expect(contextView.bindings).to.not.containEql(abcBinding);
expect(await contextView.values()).to.eql(['BAR', 'XYZ', 'FOO']);

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

Hmm, doesn't reset mean BAR and FOO should be removed?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

reset() cleans up the cache to force contextView.values() to pick up latest bindings.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

I renamed reset to be refresh to avoid confusions.

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

@raymondfeng Great refresh sounds more clear to me. And no confusions any more when I read the implementation code.

@raymondfeng raymondfeng force-pushed the context-watcher branch 2 times, most recently from 9e2e739 to e5340fd Feb 12, 2019

*/
export class ContextView<T = unknown> implements ContextObserver {
protected _cachedBindings: Readonly<Binding<T>>[] | undefined;
protected _cachedValues: T[] | undefined;

This comment has been minimized.

@jannyHou

jannyHou Feb 12, 2019

Contributor

Would it be too strict to assume that the values are all in the same type?

This comment has been minimized.

@raymondfeng

raymondfeng Feb 12, 2019

Author Member

No. We can relax as follows:

ContextView<Type1 | Type2>

@raymondfeng raymondfeng force-pushed the context-watcher branch from e5340fd to bf5cf26 Feb 14, 2019

@bajtos
Copy link
Member

bajtos left a comment

Re-posting two older comments that haven't been addressed yet. I did not look at the new patch yet.

Show resolved Hide resolved packages/context/src/context-view.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/acceptance/context-view.acceptance.ts Outdated
@bajtos
Copy link
Member

bajtos left a comment

The pull request is much easier to review and it looks pretty good now.

My major complain is about the organization of tests. They seem to be organized around the implementation details (new injection flavors are using ContextView under the hood) instead of being organized around the user experience & use cases (inject by filter, inject a getter by filter, inject a view).

Show resolved Hide resolved packages/context/src/__tests__/acceptance/context-view.acceptance.ts Outdated
Show resolved Hide resolved packages/context/src/__tests__/acceptance/context-view.acceptance.ts Outdated
}

class MyControllerWithView {
@inject.view(filterByTag('prime'))

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

I feel these three flavours @inject(filter), @inject.getter(filter) and @inject.view(filter) are sufficiently different for each other that their tests should be split into standalone describe blocks.

This test file is called context-view.acceptance.ts, so I think only @inject.view tests should stay in this file. Could you please move tests for @inject(filter) and @inject.getter(filter) to a place that's closer to other @inject and @inject.getter tests? I think class-level-bindings.acceptance is a good candidate.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Author Member

All these @inject.* that takes a binding filter function is backed by context view. If we want to move, we probably should have a dependency-injection.acceptance test. Let's leave it out of this PR.

function givenPrimeNumbers() {
givenServerWithinAnApp();
givenPrime(server, 3);
givenPrime(app, 5);

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

I don't see the connection between prime numbers and server/app-level bindings, the decision which numbers are bound to server and which to app seems pretty random to me. Can we find test data that will be easier to reason about? For example, we can use datasources and controllers, which is a concept that most LB4 developers should be already familiar with.

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Author Member

I changed it to be WorkloadMonitor.

Show resolved Hide resolved packages/context/src/__tests__/unit/context-view.unit.ts
Show resolved Hide resolved packages/context/src/inject.ts Outdated
Show resolved Hide resolved packages/context/src/inject.ts
) {
const targetType = inspectTargetType(injection);

if (targetType !== Array) {

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

Can we move this check directly to @inject? (It depends on whether design:type metadata is set before the inject is invoked.)

This comment has been minimized.

@raymondfeng

raymondfeng Feb 15, 2019

Author Member

No. injection is not available inside inject function.

@@ -100,7 +101,8 @@ function resolve<T>(
return injection.resolve(ctx, injection, s);
} else {
// Default to resolve the value from the context by binding key
return ctx.getValueOrPromise(injection.bindingKey, {
const key = injection.bindingSelector as BindingAddress;

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

Can you add an assert to verify that assumption at runtime please?

const selector = injection.bindingSelector;
if (!isBindingAddress(selector)) {
  throw new AssertionError(...);
}
const key = selector; // no cast is needed
@@ -100,7 +101,8 @@ function resolve<T>(
return injection.resolve(ctx, injection, s);
} else {
// Default to resolve the value from the context by binding key
return ctx.getValueOrPromise(injection.bindingKey, {
const key = injection.bindingSelector as BindingAddress;

This comment has been minimized.

@bajtos

bajtos Feb 15, 2019

Member

Would it perhaps make more sense to simply move the code from resolveValuesByFilter directly into resolve, so that @inject(filter) does not need any custom resolver?

@raymondfeng raymondfeng force-pushed the context-watcher branch from bf5cf26 to b5ae9a8 Feb 15, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 15, 2019

Can you add an assert to verify that assumption at runtime please?

Added.

Would it perhaps make more sense to simply move the code from resolveValuesByFilter directly into resolve, so that @inject(filter) does not need any custom resolver?

Not really, inject.ts has some utilities to check design:type. A custom resolver is used for other cases too.

@raymondfeng raymondfeng force-pushed the context-watcher branch from b5ae9a8 to 2dac020 Feb 15, 2019

@raymondfeng

This comment has been minimized.

Copy link
Member Author

raymondfeng commented Feb 15, 2019

@bajtos PTAL.

@raymondfeng raymondfeng force-pushed the context-watcher branch from 2dac020 to b71eedb Feb 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment