-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
chore(medusa): Cart service/repo cleanup #2026
Conversation
|
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) |
on hold for now till I come up with something for the naming (already got an idea, need to test it out) |
3a53224
to
f5137fb
Compare
2128e26
to
06f02af
Compare
There is an additional reason for creating |
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 Let le know Wdyt? |
Also, another interesting point is that we are using 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 |
Since newer version of typeorm, it is now possible to specify the loading strategy
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. |
Close in favour of the migration to typeorm 3 + the architecture changes |
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