sql viaJunctor changed so no duplicate rows are returned

[Benny-]

This should solve balderdashy/sails-postgresql#116

Postgresql is not the only affected adapter, the mysql adapter will benefit from this patch too.

[tjwebb]

Would you mind commenting on the lines you changed to explain what's going on? It's sort of difficult to parse the diff due to the string concatenation.

[Benny-]

I think the change is best viewed if we compare the outputed sql:

The generated statement is part of a n:m relation join using a join table.

This is a example sql statement generated before the patch:

SELECT `tag`.`name`,`tag`.`id`,`tag`.`createdAt`,`tag`.`updatedAt`,`emote_tags__tag_emotes`.`emote_tags` AS "___emote_tags"
FROM `tag` AS `tag`
INNER JOIN
    `emote_tags__tag_emotes` ON `emote_tags__tag_emotes`.`tag_emotes` = `tag`.`id`
    WHERE `tag`.`id` IN (
        SELECT `emote_tags__tag_emotes`.`tag_emotes`
        FROM `emote_tags__tag_emotes`
        WHERE `emote_tags__tag_emotes`.`emote_tags` = 218
    )
ORDER BY `tag`.`id`
ASC;

This is a example sql statement generated with the patch applied:

SELECT `tag`.`name`,`tag`.`id`,`tag`.`createdAt`,`tag`.`updatedAt`,`emote_tags__tag_emotes`.`emote_tags` AS "___emote_tags"
FROM `tag` AS `tag`
INNER JOIN (
    SELECT `emote_tags__tag_emotes` .`tag_emotes`, `emote_tags__tag_emotes` .`emote_tags`
    FROM `emote_tags__tag_emotes`
    WHERE `emote_tags__tag_emotes`.`emote_tags` = 218
) as `emote_tags__tag_emotes`
ON `emote_tags__tag_emotes`.`tag_emotes` = `tag`.`id`
ORDER BY `tag`.`id`
ASC;

The biggest difference is the removal of the WHERE tag.id IN.

[Benny-]

I now see a potential bug in the written patch.

The if(parsedCriteria) { ... } line might add something to the sql statement and depends on the removed WHERE clause.

[Benny-]

I think the introduced bug can be fixed if we change the following:

      if(parsedCriteria) {

        // If where criteria was used append an AND clause
        if(stage2.criteria.where && _.keys(stage2.criteria.where).length > 0) {
          queryString += 'AND ';
        }

        queryString += parsedCriteria.query;
      }

into

      if(parsedCriteria) {

        // If where criteria was used append an WHERE clause
        if(stage2.criteria.where && _.keys(stage2.criteria.where).length > 0) {
          queryString += ' WHERE ';
        }

        queryString += parsedCriteria.query;
      }

I prefer to run some kind of test suite before introducing any more bug. Are there any?

[Benny-]

This pull request will probably solve issue #14 too.

[mikermcneil]

@Benny @tjwebb thanks guys, we're taking a look

[mikermcneil]

@Benny- we're going to set up a basic test suite for this module-- would you mind adding a test as a separate PR that demonstrates the currently failing behavior? We've got a ton of issues/PRs we're going through right now and it would help us out a lot. Really appreciate it.

[Benny-]

Sure. I will wait until you have set up a basic test suite before adding this specific test as I am not yet sure how the test should look like.

[devinivy]

What is the status of this?

[gotmikhail]

Adding to this
balderdashy/sails#2850

Also, directly relates to #14

I agree w/ @Benny-'s fix.

Before finding this thread I initially implemented this to try it out:

queryString += ' FROM ' + utils.escapeName(stage2.child, self.escapeCharacter) + ' AS ' + utils.escapeName(stage2ChildAlias, self.escapeCharacter) + ' ';
queryString += ' INNER JOIN ' + utils.escapeName(stage1.child, self.escapeCharacter) + ' ON ' + utils.escapeName(stage2.parent, self.escapeCharacter);
queryString += '.' + utils.escapeName(stage2.parentKey, self.escapeCharacter) + ' = ' + utils.escapeName(stage2ChildAlias, self.escapeCharacter) + '.' + utils.escapeName(stage2.childKey, self.escapeCharacter);
queryString += ' WHERE ' + utils.escapeName(stage1.child, self.escapeCharacter) + '.' + utils.escapeName(stage1.childKey, self.escapeCharacter);
queryString +=  ' = ^?^ ';

This does eliminate the bug @Benny- mentioned if parsedCriteria is found, but it also means a JOIN on the entire table instead of a subset. I believe the subset will be much better.

@mikermcneil Any updates on merging in these changes? I think this is actually a pretty big issue. I mentioned a temporary fix in balderdashy/sails#2850, where you can change the defaultLimit to something large. However, as soon as you place a limit in the query string that fix is ineffective if the number of matches is over the limit.

[PatriotBob]

Is there any chance of seeing this be accepted any time soon? Not being able to populate associations reliably without having to strong arm the limit to some ridiculous value is a bit of a game stopping bug. Are we still waiting on that test suite or a specific test to be added?

[devinivy]

Yes, we'll need some tests. I would very much like this to go through too...

[gotmikhail]

@PatriotBob As I said before, as soon as you place a limit in your query string the defaultLimit solution no longer applies.

This also breaks many-to-many blueprints as well.

Let's say we have a many-to-many between Books and Readers. Say reader.name sandy has 5 books, reader.name pam has 4, and they have a single book in common. If I want to search via the blueprint,
GET - /book/5/readers?where={"name":"sandy"}&limit=1

With the current way it's implemented, you get a copy of each Reader for every book they have, then limit=1 is applied, and you only get the 1st Reader entry, pam. The query then applies the where, there's no match, so you get back an empty Array.

[Benny-]

I have no time in the near future to write any tests or do any other kind of development. If tests are required, someone else will have to write them.

[Benny-]

Can this pull request be merged or closed? I'd like to remove my fork.