Skip to content

Diogo doreto query relations #40

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

Closed
wants to merge 11 commits into from
Closed

Conversation

raymondfeng
Copy link
Contributor

@raymondfeng raymondfeng commented Mar 22, 2016

No description provided.

@kolach
Copy link

kolach commented Mar 22, 2016

That fixes should work. I'm only uncomfortable about the SQL syntax for joins.

SELECT DISTINCT child.id, child.name FROM child INNER JOIN (SELECT id FROM parent WHERE id = ? ORDER BY id) AS parent ON child.parent_id = parent.id

Why not:

SELECT child.id, child.name FROM child INNER JOIN parent ON child.parent_id = parent.id WHERE parent.id = ?;

But I guess if there is no any performance difference between the two options it's OK.

What is missing now is the ability to add more complex ORDER BY statement. Right now if there is a where clause of the filter that includes relations, the INNER JOIN is added. But what if I want to order by some field in relation? In that case LEFT OUTER JOIN should be added (if where clouse is not already added INNER one)

What do you think?

@raymondfeng
Copy link
Contributor Author

We need to make sure that the related model is backed by the same database too. Otherwise JOIN will not work.

@raymondfeng raymondfeng force-pushed the DiogoDoreto-query-relations branch from fe3f993 to c85c3b9 Compare March 23, 2016 05:45
@raymondfeng
Copy link
Contributor Author

@kolach Do you want to further enhance the PR based on your comment?

@kolach
Copy link

kolach commented Mar 23, 2016

@raymondfeng I worked on draft of what I described yesterday in my fork. I have it working now for any deepness of belongsTo relation. Here is a small example:

Models:

    Customer = ds.createModel('customer', {
      name: {
        id: true,
        type: String,
        testdb: {
          column: 'NAME',
          dataType: 'VARCHAR',
          dataLength: 32
        }
      },
      vip: {
        type: Boolean,
        testdb: {
          column: 'VIP'
        }
      },
      address: String
    }, {
      testdb: {table: 'CUSTOMER'}
    });

    Store = ds.createModel('store', {
      id: {id: true, type: String},
      state: String
    });

    Retailer = ds.createModel('retailer', {
      id: {id: true, type: String},
      name: String
    });
    // Relations
    Customer.belongsTo(Store, {as: 'store', foreignKey: 'store_id'});
    Store.belongsTo(Retailer, {as: 'retailer', foreignKey: 'retailer_id'});

Now the tests to demo:

  it('builds SELECT with deep inner join', function() {
    var sql = connector.buildSelect('customer',
      {order: 'name', limit: 5, where: {store: {retailer: {name: 'oxxo'}}}});
    expect(sql.toJSON()).to.eql({
      sql: 'SELECT `CUSTOMER`.`NAME`,`CUSTOMER`.`VIP`,`CUSTOMER`.`ADDRESS`,' +
      '`CUSTOMER`.`STORE_ID` FROM `CUSTOMER`' +
      ' INNER JOIN `STORE` ON `CUSTOMER`.`STORE_ID`=`STORE`.`ID`' +
      ' INNER JOIN `RETAILER` ON `STORE`.`RETAILER_ID`=`RETAILER`.`ID`' +
      ' WHERE `RETAILER`.`NAME`=$1 ORDER BY `CUSTOMER`.`NAME` LIMIT 5',
      params: ['oxxo']
    });
  });

  it('builds SELECT with left inner and outer joins because', function() {
    var sql = connector.buildSelect('customer',
      {where: {store: {state: 'NY'}}, order: {store: {retailer: 'name'}}, limit: 5});
    expect(sql.toJSON()).to.eql({
      sql: 'SELECT `CUSTOMER`.`NAME`,`CUSTOMER`.`VIP`,`CUSTOMER`.`ADDRESS`,' +
      '`CUSTOMER`.`STORE_ID` FROM `CUSTOMER`' +
      ' INNER JOIN `STORE` ON `CUSTOMER`.`STORE_ID`=`STORE`.`ID`' +
      ' LEFT OUTER JOIN `RETAILER` ON `STORE`.`RETAILER_ID`=`RETAILER`.`ID`' +
      ' WHERE `STORE`.`STATE`=$1 ORDER BY `RETAILER`.`NAME` LIMIT 5',
      params: ['NY']
    });
  });

To make it work for REST request I had to modify loopback-datasource-juggler. Method:

DataAccessObject._normalize = function (filter) {

I commented out all the logic about order parameter processing. As I need it to accept more complex objects. (But string and array are also supported)

I can make a PR, for you can review and borrow some parts of the code or ideas for better solution.

@ywang-clarify
Copy link

I think one thing that the nested subqueries approach has going for it is that the aliases are scoped to the subquery. So if you need to limit child by grandparent properties, the join to grandparent would not be visible at the SQL query scope of the child. This could help in avoiding duplicate aliases.

Consider a query for a child who's mother's father's name is Bob and father's father's name is Steve:
filter={"where":{"mother":{"father":{"name":"Bob"}},"father":{"father":{"name":"Steve"}}}}

In this case, the tables/aliases for mother's father and the father's father don't end up colliding.

I do want to point out that when creating aliases, they probably need to be be aliased based on the relation name, not the table name. In the above example, if "mother" and "father" were both relations that lead to an instance in a "person" table, you'd end up writing some bad SQL unless you gave them distinct aliases.

If you want to pursue the approach w/o nested subqueries, you'd probably have to generate join aliases based on the path of relationships traversed. So in the above example, you'd have joins aliased as "mother", "father", "mother_father", "father_father" or something like that.

Also, I don't like the DISTINCT in the subqueries approach. It causes problems for me in the Postgres connector where there're JSON columns and DISTINCT isn't supported for them because there isn't an equality comparator. Maybe I'm missing something, but I don't quite see why it was deemed necessary in that approach.

@LoicMahieu LoicMahieu mentioned this pull request Apr 5, 2016
@Amir-61
Copy link
Contributor

Amir-61 commented Apr 6, 2016

Connect to : #31

Related to: #42 #44

Related GH issue: strongloop/loopback#517

@stale
Copy link

stale bot commented Aug 22, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot closed this Sep 5, 2017
@0candy 0candy removed the backlog label Sep 5, 2017
@stale
Copy link

stale bot commented Sep 5, 2017

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants