feat(core): add support for alternative loading strategies #556
Conversation
|
This pull request introduces 2 alerts when merging 9b09658 into 677c9e3 - view on LGTM.com new alerts:
|
|
Can't push to your fork, not sure why. Here is what I did to fix those 2 tests. But for me on local host few more tests are failing, not sure why (namely Index: packages/core/src/EntityManager.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/core/src/EntityManager.ts (revision c134f3e9b1f086570d40944f9d610c7d078c8512)
+++ packages/core/src/EntityManager.ts (date 1589031966559)
@@ -586,13 +586,16 @@
}
const preparedPopulate = this.preparePopulate(entityName, options.populate);
-
await this.entityLoader.populate(entityName, [entity], preparedPopulate, where, options.orderBy || {}, options.refresh);
return entity;
}
private preparePopulate(entityName: string, populate?: string | Populate): PopulateOptions[] {
+ if (populate === true) {
+ return [{ field: '*', all: true }];
+ }
+
const meta = this.metadata.get(entityName);
if (Array.isArray(populate)) {
Index: packages/core/src/drivers/IDatabaseDriver.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/core/src/drivers/IDatabaseDriver.ts (revision c134f3e9b1f086570d40944f9d610c7d078c8512)
+++ packages/core/src/drivers/IDatabaseDriver.ts (date 1589031926857)
@@ -103,4 +103,5 @@
export type PopulateOptions = {
field: string;
strategy?: LoadStrategy;
+ all?: boolean;
};
Index: packages/core/src/entity/EntityLoader.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- packages/core/src/entity/EntityLoader.ts (revision c134f3e9b1f086570d40944f9d610c7d078c8512)
+++ packages/core/src/entity/EntityLoader.ts (date 1589031919483)
@@ -34,7 +34,7 @@
}
private normalizePopulate(entityName: string, populate: PopulateOptions[] | true, lookup: boolean): PopulateOptions[] {
- if (populate === true) {
+ if (populate === true || populate.some(p => p.all)) {
populate = this.lookupAllRelationships(entityName);
} else {
populate = Utils.asArray(populate); |
|
Btw I believe there are some unresolved comments in the previous PR, right? Like the aliasing of joined tables ( |
|
Thanks! I’ll integrate those changes into my fork...not sure why you can’t push since I’ve allowed edits from maintainers. and yes, still pending items. I’ve been focused on getting all existing tests passing so I can write more joined load tests |
|
This pull request introduces 2 alerts when merging ef94fd7 into ff77f9d - view on LGTM.com new alerts:
|
|
Referencing #517 (comment), did you have any implementation suggestions? I assume you mean to use the existing I am currently relying on the order of It's less clear to me how I would use the |
|
Maybe reuse |
|
So, I agree that relying on index is brittle and I have been familiarizing myself with the code you suggested. I do have a couple of questions though:
I definitely feel like the pieces are there but I'm not sure how much you expect to change (eg, should most of the logic move into the Also, thanks for your patience and help on this, I know you probably could have had this done already. It's been nice to learn a small piece of the codebase though. |
QB is using the driver to map queries, so its fine that the mapping part is in driver. But the ultimate goal is to have QB also usable "alone", it will still use driver for executing and mapping the results to entities, but we should be also able to use this new joined strategy with query builder, without using entity manager. That basically means that the internal const authors = await qb.select().populate(['books']).getResult();
// authors[0].books[0] should be populated Book instance via joined strategy, regardless of the books property settingsBut that could be delivered as part of another PR (not necessarily by you), as a follow up.
I think I mentioned that it would need to be adjusted by making the That
I think this should be now answered already, if not, let's discuss concrete parts of the implementation. But the mapping to entities needs to stay in the driver. |
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
|
What's the status? I'd like to merge this soon, it does not have to be 100% completed just yet, as long as the tests are passing and we are back on 100% coverage I think we can merge (we are targeting dev branch so not a big deal if something is broken). I can imagine the pain of rebasing this (and I am holding my horses not rebasing dev with latest changes from master, so trying to make it less painful 😄). It would be probably good for both of us to have this merged asap and define the list of missing things somewhere so we have a clear understanding of where we are, and do them as (maybe more than one) smaller PR(s) on top of this. I'd like to work on lazy scalar properties next week and I can imagine a lot of merge conflicts If I do it before the core of this is merged. So would be great if we can get this to mergable state soon (but no rush, and thanks for all the work on this!) |
|
Sorry! I was planning on working on the last item you commented on today. The week has been really busy so didn’t get around to it like I’d hoped. So we can get it merged, I can work on getting test coverage back to 100% instead and then, like you said, have follow-up PRs with fixes and improvements. I know there is missing stuff but I’m starting to feel bad it’s been open for so long lol Do you prefer I get that last comment addressed and get test coverage back to 100 or just get coverage back? |
|
No need to be sorry :] I also had quite busy working week... And no need to rush, I would aim to merge this early next week, but its up to you and your capacity. Would be good to address that aliases thing, if that is what you mean, then I would probably rather wait for that to be implemented. |
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
|
This pull request introduces 1 alert when merging dc0dc79 into 3b3eec7 - view on LGTM.com new alerts:
|
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
|
This pull request introduces 1 alert when merging 9293cbe into 3b3eec7 - view on LGTM.com new alerts:
|
This ensures that even if no related records are returned by the joined load, we still end up with the root entity. Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
|
@B4nan Okay, so here is where I am at:
WRT coverage, I am not sure what else I can do. Coveralls is not reporting any uncovered lines like it has been so I don't know what else to add 😬 |
It's about branches, not just lines. Also if you used optional chaining somewhere, we might need to ignore that in coverage report via Run |
Signed-off-by: Will Soto <willsoto@users.noreply.github.com>
|
@B4nan not sure if you get notified on every push, but CI is now passing so you can take a look whenever you get a chance. |
|
Ok let's merge this and improve via other PRs. Sorry about the confusion with Anyway, thanks again! |
B4nan
changed the title
|
@B4nan Agreed. I considered passing around the query builder at one point for exactly that reason but I also wondered if there was just a better api in general. In hindsight I probably should have made Feel free to reach out once you are done with some of the other work you have in mind. I would love to continue working on this - albeit with smaller more manageable PRs 😆 Thanks again for all the feedback and patience |
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported: `joined`. Currently, the only way to specify the strategy is on on the decorator for the relationship: ```ts @entity() export class Author2 { @OneToMany({ entity: () => Book2, mappedBy: 'author', orderBy: { title: QueryOrder.ASC }, strategy: LoadStrategy.JOINED, // new property }) books!: Collection<Book2>; } ``` Related: #440
This adds support for alternate load strategies. As of now, only one additional strategy is supported:
joined. Currently, the only way to specify the strategy is on on the decorator for the relationship:Closes #440 (reopen of #517)