balderdashy / waterline-sequel

A SQL generator for use in Waterline Adapters
MIT License
16 stars 61 forks source link

Keepin' it simple in the complex query. Simplify join. #54

Closed devinivy closed 9 years ago

devinivy commented 9 years ago

Proposed fix for #53. This seems to fix the issue, and all adapter tests remain passing. That said, I'd like someone who knows exactly when this query is used and how it is used to make sure this is a valid change. The query has become much simpler, and (of course) it is not equivalent to the existing query.

Painting in broad strokes, I'm changing

SELECT files.* FROM files
    INNER JOIN user_has_file ON user_has_file.fileId=files.id
    WHERE files.id IN
        (SELECT user_has_file.fileId FROM user_has_file
            WHERE user_has_file.userId = 1)
    LIMIT 10;

to

SELECT files.* FROM files
    INNER JOIN user_has_file ON user_has_file.fileId=files.id
    WHERE user_has_file.userId = 1
    LIMIT 10;

But other criteria can be used with this query, and I'm not sure if that's meant to work in some magical way with the existing query.

This is a hot fix, but I don't want to rush any junk code into production :P. CC waterline-sequelers @particlebanana @RWOverdijk

Edit Query introduced as part of this commit: https://github.com/balderdashy/waterline-sequel/commit/65376387abbf7340b669f4aa2642d36dc8783e9a. Maybe @particlebanana recalls the original purpose.

dmarcelino commented 9 years ago

Looks alright to me but I'm wondering what could be the purpose of the previous query...

@RWOverdijk, I remember you getting your hands quite dirty with some waterline-sequel queries, what do you think of this? Thanks

particlebanana commented 9 years ago

So the reason we have those sub queries is skip, sort, limit on populations. So given the following query:

User.find({ name: 'foo' })
.populate('pets', { type: 'dog', skip: 10, limit: 2, sort: 'age desc' })
.exec(/*...*/)

You will need to run that query in the populate for each user found in the parent query. Otherwise you will get the skip, limit, sort applied to all.

I haven't looked at #53 though so let me look at the failing test and see whats up.

devinivy commented 9 years ago

The exact issue is that sort/limit/skip do not play nicely with the existing query. Please check out #53 for an example explaining why the existing query doesn't work.

particlebanana commented 9 years ago

Hmm yeah I see, to be honest I don't remember why that IN query exists in the first place as it doesn't seem to be doing anything. Let me keep digging.

devinivy commented 9 years ago

Tomorrow this patch will have been live on prod for a week. We haven't had any issues!

particlebanana commented 9 years ago

Ok, I'll look at getting a version published today with some of the fixes we implemented.