chore(medusa): Cart service/repo cleanup

[adrien2p]

Proposal for discussion

Probably require the migration to typeorm 0.3 to continue and validate this one around a discussion

What

After discussing with @olivermrbl I got the information that in the custom cart repository, the way the find with relations has been re implemented was due to the fact that the generated sql query was too big.

After interrogating my self i was wondering how a query can be more that 1GB which is the limit of the allocated memory for a query string.

I also find out that in the same cart service, the list method was using the find method without using the internal re implementation and that as of now we did not get any problem.

So I decided to test it and everything is working as expected, which also re allow the original features that typeorm provides such as being able to filter on the relations as well as on the entity itself.

Also, the usage of custom methods, does not allow to filter the query against the relations, or just selecting some columns on those relations to not increase the memory with data that are not used, or other internal feature provided by typeorm

Test

The actual integration tests for the totals service already tests that, it includes 16 relations and select all the fields of the cart as well as of all the relations

Feedback

I might miss some other information or context that I would like to be aware of if that is the case.

Include FIXES CORE-428

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 06f02af4517492095f253050cef6ca4aff042325

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[olivermrbl]

comment: Note, this does not solve the issue with long generated table name aliases, which was why we chose to build our way of retrieving relations. See this issue for more details.

(Have described it in CORE-428 as well)

[adrien2p]

on hold for now till I come up with something for the naming (already got an idea, need to test it out)

[srindom]

There is an additional reason for creating findWithRelations. We encountered OOM errors when the default TypeORM query, could imagine the reason being that there was an enormous amount of data being returned when many relations were joined (due to the cartesian product of one to many relations).

[adrien2p]

There is an additional reason for creating findWithRelations. We encountered OOM errors when the default TypeORM query, could imagine the reason being that there was an enormous amount of data being returned when many relations were joined (due to the cartesian product of one to many relations).

I suppose that this behaviour would also happen in the actual usage since we do not reduce the amount of data when we aggregate them? It is still the same amount of data that we have in the output object from the custom method. wdyt? sorry I supposed it to be from node, but I suppose you talk about postgres 😂

If it happen to be the case, it means that we are fetching too much and also means that it should be cut down at another level. For example the service could retrieve the cart with some relations for its usage, then retrieving the cart with other relations for another usage etc.

I think that by doing a custom method like that we are removing some features provided by typeorm and could miss some feature improvement on version bump (performance, query generation , etc).

It might also be related to postgres configuration such as the number of connections allowed simultaneously, the memory allowed per connection, the work_mem allocated for each connection, the machine memory, the vaccum, and so on

Let le know Wdyt?

[adrien2p]

Also, another interesting point is that we are using find in the list method and the custom one in the retieve. I would have assumed that the list method would fetch (or allowed to fetch) more data than the retrieve method. And i ve taken that into account in my analysis 😅

I think that it is something that might need to be discussed in a meeting to get more in depth of the different thinking here 🚀

In the mean time i ll do some tests on my side to push the limits

[adrien2p]

Since newer version of typeorm, it is now possible to specify the loading strategy

• relationLoadStrategy https://github.com/typeorm/typeorm/pull/8616

Of course there is few breaking changes but nothing we can't tackle i suppose 🤔

I will probably use that pr for my DD and attempt a migration to the last typeorm version while migrating the code in a way that it does not require to update everything. In a second step we would be able in those DD to start to simplify our repositories.

[adrien2p]

Close in favour of the migration to typeorm 3 + the architecture changes