Skip to content
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

[Bug]: Mysql error "LIMIT in subquery" #77

Closed
axelprat opened this issue Apr 6, 2020 · 9 comments
Closed

[Bug]: Mysql error "LIMIT in subquery" #77

axelprat opened this issue Apr 6, 2020 · 9 comments
Labels
good first issue Good for newcomers

Comments

@axelprat
Copy link

axelprat commented Apr 6, 2020

Hi @doug-martin ,

Context

I'm using nestjs-query to easily implement graphql in my company and this project is really great and usefull !!

In this project we have multiple endpoints and multiple databases (all in mysql 5.7). Currently we are working on one main endpoint and some entities from other databases.

I made a repo to reproduce the issue : https://github.com/praaxe/nestquery-example .
You just need to update config/config.ts and run src/commands/generate-fixture.ts to have some data. (beware it deletes all tables in the database ><)

The issue is when querying sub-relations I have a sql error

ER_NOT_SUPPORTED_YET: This version of MySQL doesn't yet support 'LIMIT & IN/ALL/ANY/SOME subquery'

image

In the repo I added a patch I did to bypass this error in the resolvers' comments but I didn't find how to use dataloader so this ins't really good.

Patching

I found a way around by overiding 1-n relationships in resolvers and adding (paging: {}) to n-* sub-relations but I don't really like it and it would be better to find a definitive correction.

image

Versions

  • Mysql: 5.7
  • nestjs: 7.0.7
  • typeorm: 7.0.0
  • nestjs-query: 0.7.4

ps: thank you again for this project !

@doug-martin doug-martin added the good first issue Good for newcomers label Apr 6, 2020
@doug-martin
Copy link
Owner

@praaxe thank you for the detailed issue write up!

I'm starting to think better approach for the connections may be to not do sub queries, since it is flakey across databases and versions, and I dont want to have to try to detect DB flavors and versions as that belongs in the persistence layer (e.g. Typeorm).

We could still use dataloader so the queries only run once but it wont issue a single query for all connections.

Thoughts?

@axelprat
Copy link
Author

axelprat commented Apr 6, 2020

I think this would be a better approach indeed. With dataloader it won't break down the database and each request will be very efficient.

We did it in a previous test implementation and dataloader really did the job to resolve n+1 issue.

@axelprat
Copy link
Author

axelprat commented Apr 7, 2020

Moreover I encountered an issue with entities from another database where I couldn't declare the relation in the entity because the library didn't find the second entity in the connection of the first entity.

With new queries instead of sub-queries this could be addressed too =)

@doug-martin
Copy link
Owner

Do relations in different DBs work in TypeOrm without nestjs-query? I can see one to one may work but I'm not sure how many to many would work.

@axelprat
Copy link
Author

axelprat commented Apr 7, 2020

Actually I just implemented one side of the relations (from the main database to the second) and I couldn't declare the relation in entity so I did it in the resolver.

I will update the example project to implement a second database and a second module like I did for my company to show you.

@axelprat
Copy link
Author

axelprat commented Apr 7, 2020

Project is now updated with a second database.
Pull the new version, re-run fixtures and you're good to go.

I commented the instantiation of graphql in second.module.ts because we use only one endpoint with ReadResolver for entities from other database but if you want to uncomment it both endpoints will work just fine.

doug-martin added a commit that referenced this issue Apr 9, 2020
* [FIXED] Mysql error "LIMIT in subquery"
  * Changed `nestjs-query/query-typeorm` to not use subqueries to fetch relations.
doug-martin added a commit that referenced this issue Apr 9, 2020
* [FIXED] Mysql error "LIMIT in subquery"
  * Changed `nestjs-query/query-typeorm` to not use subqueries to fetch relations.
@doug-martin
Copy link
Owner

@praaxe I fixed the issue with limit in sub queries in v0.8.1 let me know if you have any issues.

For relations that are not in the same DB, that wont currently work since the relations are based on what typeorm knows about. This isnt ideal but I'll need to think about how do make relations decoupled from the persistence layer.

@axelprat
Copy link
Author

axelprat commented Apr 9, 2020

I just updated my project and it work just fine, thanks @doug-martin !

And yes, I just realized that the error I get come from TypeOrm. Don't worry too much about that, this isn't an usual behaviour and it works anyway.

There is just one thing that I saw with the new version about dataloader, you use it to batch the queries but when you have all the queries to run you still make multiple calls (example @nestjs-query/query-typeorm/dist/src/services/relation-query.service.js line 86 I believe).
I don't know if it is a wanted behaviour but it may be stressfull for the database if we go through multiple relations

@doug-martin
Copy link
Owner

@praaxe yeah I don't like that we cant batch those up, but with the limitation you ran into with mysql 5.7 I haven't found a clean way to do it without subqueries since each query has its own paging params and sorting params that cant be applied to the main query.

The separate queries are cached with dataloader and subsequent calls within the same request should not be executed twice.

If you have a suggestion I'd love to hear it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants