-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[WIP] feat(repository): add KVRepository impl using legacy juggler #1527
Conversation
a86a764
to
2138def
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great start!
Please add tests to cover all newly added APIs, see packages/repository/test/unit/repositories/legacy-juggler-bridge.unit.ts.
I am concerned about the growing size of legacy-juggler-bridge.ts
file, is it perhaps time to split it into multiple files, one for each different repository?
@@ -38,7 +35,7 @@ export interface KVRepository<T extends Model> extends Repository<T> { | |||
* @param options Options for the operation | |||
* @returns A promise of the entry | |||
*/ | |||
get(key: string, options?: Options): Promise<T>; | |||
get(key: string, options?: Options): Promise<DataObject<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, repositories should always return model instances, not data objects. Our CRUD repository already does that (see e.g. findById
method), let's do the same for KeyValue too please.
*/ | ||
expire(key: string, ttl: number, options?: Options): Promise<boolean>; | ||
expire(key: string, ttl: number, options?: Options): Promise<number>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect the returned TTL set to be different from the ttl requested? How do you expect the callers to handle that situation? If the returned TTL is always the same as requested, then what's the benefit of returning that value?
I think it would be better to change this API to return Promise<void>
.
Thoughts?
*/ | ||
iterateKeys?(filter?: Filter, options?: Options): Promise<Iterator<T>>; | ||
iterateKeys?(filter?: Filter, options?: Options): Iterator<Promise<string>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, the key-value access API provided by LB 3.x returns AsyncIterator<string>
which should be consumable via for await
in Node.js 10.x:
for await (const key of model.iterateKeys()) {
// ...
}
Could you please verify my claim and then fix the typescript definitions accordingly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LB3 does not fully implement AsyncIterator. See https://github.com/strongloop/loopback-datasource-juggler/blob/ade9f0fd9c1b133b3e6fc8dfec73fa82b5f0ab70/types/kv-model.d.ts#L210-L219
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. When I was implementing KeyValue data-access API, my intention was to design iterateKeys
in a way that make it usable as an async iterator, at least in the way how async iterators were specified at that time.
What are we missing in our implementation that makes iterateKeys
incompatible with AsyncIterator
and for-await-of
? I would like to fix it in juggler before the first release of KeyValue API in LB4.
I suppose iterateKeys
can be fixed after this initial pull request is landed in a follow-up work.
@@ -43,10 +44,12 @@ export function createHasManyRepositoryFactory< | |||
'The foreign key property name (keyTo) must be specified', | |||
); | |||
} | |||
// tslint:disable-next-line:no-any | |||
const constraint: any = {[fkName]: fkValue}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you tried using object
instead of any
, or perhaps leave out the type information altogether?
const contraint = {[fkName]: fkValue};
throw new Error('Not implemented'); | ||
} | ||
|
||
deleteAll(options?: Options): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAICT, juggler 3.x provides both delete
and deleteAll
operations, although they seems like not documented in our apidocs.
See the tests for usage examples:
@@ -68,7 +64,7 @@ export interface KVRepository<T extends Model> extends Repository<T> { | |||
* @param options Options for the operation | |||
* @returns A promise of the TTL value | |||
*/ | |||
ttl?(key: string, ttl: number, options?: Options): Promise<number>; | |||
ttl?(key: string, options?: Options): Promise<number>; | |||
|
|||
/** | |||
* Fetch all keys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1.
d23051d
to
bc0d9b1
Compare
@bajtos Please review loopbackio/loopback-datasource-juggler#1608 1st. |
bc0d9b1
to
ef5ddb9
Compare
ef5ddb9
to
ca55097
Compare
github seems to be messed with the PR - two files are missing. |
The PR adds a default implementation of KVRepository with legacy juggler
Depends on loopbackio/loopback-datasource-juggler#1608
Connects to #1448
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated