feat(repository): add KVRepository impl using legacy juggler #1539

raymondfeng · 2018-07-16T19:03:01Z

Replacing #1527, see also #1448

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

bajtos · 2018-07-17T12:39:39Z

packages/repository/src/repositories/kv.repository.bridge.ts

+  kvModelClass: typeof juggler.KeyValueModel;
+
+  constructor(kvModelClass: typeof juggler.KeyValueModel) {
+    this.kvModelClass = kvModelClass;


I think this design is not correct. IIRC, in order to create juggler.KeyValueModel, one needs a DataSource (or at least a ModelBuilder). That's incompatible with LB4 design, where model classes are standalone and decoupled from repository implementation (legacy juggler bridge or other).

See how DefaultCrudRepository takes a LB4 model definitions and creates a backing LB3 PersistedModel instance:

https://github.com/strongloop/loopback-next/blob/4b14a5ca46ff542002bdbb36c8986f79cc33a5e1/packages/repository/src/repositories/legacy-juggler-bridge.ts#L98

https://github.com/strongloop/loopback-next/blob/4b14a5ca46ff542002bdbb36c8986f79cc33a5e1/packages/repository/src/repositories/legacy-juggler-bridge.ts#L101-L125

IMO, we should do the same for KeyValue models too.

bajtos · 2018-07-17T12:40:53Z

packages/repository/src/repositories/kv.repository.bridge.ts

+  }
+
+  get(key: string, options?: Options): Promise<T> {
+    const val = this.kvModelClass.get(key, options) as legacy.PromiseOrVoid<T>;


This is not a valid cast. Typically, T is a subclass of the new LB4 Model class, while kvModelClass.get returns an instance of legacy LB3 KeyValueModel class. (See also my comment above.)

This is not a valid cast. Typically, T is a subclass of the new LB4 Model class, while kvModelClass.get returns an instance of legacy LB3 KeyValueModel class. (See also my comment above.)

☝️ this is an important point to address or discuss ☝️

You can take a look at how (CRUD) Repository deals with conversion between backing-model instances and LB4 model instances.

bajtos · 2018-07-17T12:43:21Z

packages/repository/src/repositories/kv.repository.bridge.ts

+    return ensurePromise<string[]>(this.kvModelClass.keys(filter, options));
+  }
+
+  iterateKeys(filter?: KVFilter, options?: Options): AsyncIterator<string> {


In the previous pull request, I proposed to drop keys(...): Promise<string[]> and rename iterateKeys to keys. What are your arguments for keeping both flavors?

Cross-posting #1527 (comment):

Loading all keys into memory is a foot-gun that opens doors to a DoS attack where the datasource is queried with a pattern that matches too many keys.

The only reason why I added the method keys(): string[], was to work around the limitations of LB 3.x - the way how it was directly exposing juggler methods via REST APIs and a lack of support for streaming responses.

We don't have that limitation in LB4 because REST API is exposed via custom Controller classes that can work around missing support for streaming responses in LB4.

I am proposing to remove keys(): string[] API from LB4 and provide only one method based on AsyncIterator. Ideally, this single method should be called keys instead of iterateKeys for simplicity.

Until our REST layer supports AsyncIterators, we can provide a helper function to convert AsyncIterator into a static array that people can use in their controllers.

Thoughts?

I am ok to leave this change out of this pull request if you prefer, as long as we make it happen before the first public release of KeyValue feature in LB4.

bajtos · 2018-07-17T12:46:06Z

packages/repository/test/unit/repositories/kv.repository.bridge.unit.ts

+      name: 'db',
+      connector: 'kv-memory',
+    });
+    kvNoteModel = ds.createModel<typeof juggler.KeyValueModel>('note');


IMO, users should not need to understand these low-level implementation details of the legacy juggler bridge. Instead, they should be able to use the same API as DefaultCrudRepository offers now.

repo = new DefaultKVRepository<Note>(Note, ds);

bajtos · 2018-07-17T12:46:36Z

packages/repository/test/unit/repositories/kv.repository.bridge.unit.ts

+    const note1 = {title: 't1', content: 'c1'};
+    await repo.set('note1', note1);
+    const result = await repo.get('note1');
+    expect(result).to.eql(note1);


Please add an assertion that result is instanceof Note.

Please add an assertion that result is instanceof Note.

PTAL ☝️

bajtos · 2018-07-17T12:49:52Z

packages/repository/test/unit/repositories/kv.repository.bridge.unit.ts

+    await repo.set('note2', {title: 't2', content: 'c2'});
+    const keys = repo.iterateKeys!();
+    const keyList: string[] = [];
+    while (true) {


Have you considered using for-await-of? I believe TS should already support it when the lib is set to es2018.

IIRC, esnext is needed.

And we need to return an iteratable object (with a Symbol.asyncIterator property) instead.

And we need to return an iteratable object (with a Symbol.asyncIterator property) instead.

Let's do that please. It's one of the main reasons why I am pushing for AsyncIterator interface - to allow people to consume the keys via for-await-of or any async-iterable helper functions (think of lodash for async-iterables).

IIRC, esnext is needed.

You are right. I am proposing to add ESNext.AsyncIterable to our lib. Async iterators are already available on Node.js 10.0.0 with no feature flags needed.

bajtos · 2018-08-13T14:02:53Z

packages/build/config/tsconfig.common.json

    "strictNullChecks": true,

-    "lib": ["es2018", "dom"],
+    "lib": ["es2018", "dom", "esnext.asynciterable"],


bajtos · 2018-08-13T14:08:17Z

packages/repository/src/repositories/kv.repository.bridge.ts

+  }
+
+  get(key: string, options?: Options): Promise<T> {
+    const val = this.kvModelClass.get(key, options) as legacy.PromiseOrVoid<T>;


This is not a valid cast. Typically, T is a subclass of the new LB4 Model class, while kvModelClass.get returns an instance of legacy LB3 KeyValueModel class. (See also my comment above.)

☝️ this is an important point to address or discuss ☝️

You can take a look at how (CRUD) Repository deals with conversion between backing-model instances and LB4 model instances.

bajtos · 2018-08-13T14:09:06Z

packages/repository/src/repositories/kv.repository.ts

+ * Polyfill for AsyncIterator before es.next is ready
+ */
+// tslint:disable:no-any
+export interface AsyncIterator<T> {


Do we still need this poly-fill after you enabled esnext.asynciterable in tsconfig?

bajtos · 2018-08-13T14:10:18Z

packages/repository/test/unit/repositories/kv.repository.bridge.unit.ts

+    const note1 = {title: 't1', content: 'c1'};
+    await repo.set('note1', note1);
+    const result = await repo.get('note1');
+    expect(result).to.eql(note1);


Please add an assertion that result is instanceof Note.

PTAL ☝️

bajtos · 2018-08-13T14:12:12Z

packages/repository/src/repositories/kv.repository.bridge.ts

+    return ensurePromise<number>(this.kvModelClass.ttl(key, options));
+  }
+
+  keys(filter?: KVFilter, options?: Options): Promise<string[]> {


In the thread #1539 (comment), you said you agree to drop iterateKeys and rework keys to return an async iterator. However, I see the old API is still present. Perhaps an oversight?

bajtos · 2018-08-13T16:40:06Z

packages/repository/src/repositories/kv.repository.bridge.ts

+/**
+ * An implementation of KVRepository based on loopback-datasource-juggler
+ */
+export class DefaultKVRepository<T extends Model> implements KVRepository<T> {


While you are touching these APIs, could you please rename KV to KeyValue for more clarity? DefaultKeyValueRepository and KeyValueRepository.

bajtos

Two more comments, feel free to address them in a follow-up pull request.

bajtos · 2018-08-14T09:51:19Z

packages/repository/src/repositories/kv.repository.bridge.ts

+    // KVModel class is placeholder to receive methods from KeyValueAccessObject
+    // through mixin
+    this.kvModelClass = ds.createModel<typeof juggler.KeyValueModel>(
+      '_kvModel',


I wrote a comment on this line that was somehow lost :(

Consider the following scenario: an app has two key-value models called Stats and ShoppingCart, and two KV repository classes extending DefaultKeyValueRepository, both using the same backing datasources. The first instance will define _kvModel model, the second instance will re-define this model again. From what I remember from LB 3.x days, redefining model of the same name was considering as invalid usage of juggler API.

Creating a new model class on every request has negative performance impact too.

Another problem I see here is that we are not defining any properties on the backing model class. As a result, legacy juggler is not going to verify that the input data are valid model properties. AFAIK, KVAO does not implement validations yet (see lib/kvao/set.js), thus I guess this second point is not a blocker for this pull request.

Ideally, I'd like our DefaultKeyValueRepository to leverage the same mechanism that DefaultCrudRepository uses:

The backing model has the same name as the LB4 model.

If the datasource already has a backing model defined, then we reuse it.

When defining a new backing model, LB4 property definition is converted to juggler schema.

See https://github.com/strongloop/loopback-next/blob/5a6bed821fa01433c48ad437578d5a13f8a11cd8/packages/repository/src/repositories/legacy-juggler-bridge.ts#L101-L132

bajtos · 2018-08-14T09:53:25Z

packages/repository/test/unit/repositories/kv.repository.bridge.unit.ts

+    const note1 = {title: 't1', content: 'c1'};
+    await repo.set('note1', note1);
+    const result = await repo.get('note1');
+    expect(result).to.eql(new Note(note1));


Is eql comparing prototype/constructor? I would prefer an explicit instanceof check instead.

expect(result).to.be.instanceOf(Note);

raymondfeng requested a review from bajtos as a code owner July 16, 2018 19:03

bajtos suggested changes Jul 17, 2018

View reviewed changes

raymondfeng force-pushed the kv-repository branch from 30ce60e to 0b9e47b Compare July 17, 2018 17:48

raymondfeng force-pushed the kv-repository branch 4 times, most recently from 1fdaa93 to ca93e61 Compare August 3, 2018 21:29

bajtos suggested changes Aug 13, 2018

View reviewed changes

bajtos mentioned this pull request Aug 13, 2018

Spike: KeyValue repository #1448

Closed

3 tasks

bajtos reviewed Aug 13, 2018

View reviewed changes

bajtos mentioned this pull request Aug 13, 2018

[CLI] add lb4 repository command #1588

Closed

9 tasks

raymondfeng force-pushed the kv-repository branch 3 times, most recently from 12b58c1 to b41b17e Compare August 13, 2018 20:12

feat(repository): add KVRepository impl using legacy juggler

8ba0801

raymondfeng force-pushed the kv-repository branch from b41b17e to 8ba0801 Compare August 13, 2018 20:55

bajtos approved these changes Aug 14, 2018

View reviewed changes

raymondfeng merged commit 97a75dc into master Aug 14, 2018

virkt25 added this to the August Milestone milestone Aug 15, 2018

raymondfeng mentioned this pull request Aug 15, 2018

[Epic] KeyValue repository #1447

Closed

6 tasks

bajtos deleted the kv-repository branch August 23, 2018 09:52

feat(repository): add KVRepository impl using legacy juggler #1539

feat(repository): add KVRepository impl using legacy juggler #1539

Uh oh!

Conversation

raymondfeng commented Jul 16, 2018 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

raymondfeng commented Jul 16, 2018 •

edited by bajtos

Loading