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

ColumnName in the association primaryKey #6144

Closed
Mordred opened this issue Apr 2, 2014 · 11 comments
Closed

ColumnName in the association primaryKey #6144

Mordred opened this issue Apr 2, 2014 · 11 comments

Comments

@Mordred
Copy link
Contributor

Mordred commented Apr 2, 2014

Hi,

I'm trying to map sails models to the very old database tables:

// Card
attributes: {
  person:    {
    model:      'Person',
    columnName: 'people_id'
  }
}

// Person
attributes: {
  id:           {
    columnName: 'people_id',
    type:       'integer',
    required:   true,
    primaryKey: true,
    index:      true
  }
}

But I'm getting an error:

{
  "error": "E_UNKNOWN",
  "summary": "Encountered an unexpected error",
  "status": 500,
  "raw": "Error: ER_BAD_FIELD_ERROR: Unknown column 'peoples.id' in 'where clause'"
}

I think that the columnName of the Person.attributes.id is not used.

I don't know if this is waterline problem or sails-mysql. I'm using a sails-mysql 0.10.0-rc5

@jasonsims
Copy link
Contributor

+1 I'm hitting this issue also.

Specifying columnName for primary keys will break associations. Waterline seems to ignore this field on primary keys when it comes to joining. I've found that part of the issue is in query/deferred.js#L83. Here we should be doing something more like:

pk = self._context._attributes[key].columnName || key;

There's more to the problem that I cannot find the root cause to though. The joins are also not using columnName for primaryKey in child relationships. For example, the criteria passed to query/finders/basic.js#findOne includes the model key name (id) and not columnName (people_id) for "childKey" in the joins list.

You should be able to reproduce the issue with these models using any adapter:

// One Account can have many Tickets
// One ticket can have many TicketComments
// 
// When you try to run `Ticket.find().populate('account').populate('comments')` 
// the population will for both the Ticket's child relationship to Account, and 
// for the Ticket's parent relationship to TicketComment.

// Account -------------------------------------------------------------------
attributes: {
  id: {
    columnName: 'account_id',
    type: 'string',
    primaryKey: true
  },
  name: 'string',

  ////////////////////////
  // Associations
  tickets: {
    collection: 'Ticket',
    via: 'account'
  }
}


// Ticket --------------------------------------------------------------------
attributes: {
  id: {
    type: 'string',
    columnName: 'ticket_id',
    primaryKey: true
  },
  ticketNumber: 'string',

  ////////////////////////
  // Associations
  account: {
    type: 'string',
    columnName: 'account_id',
    model: 'Account'
  }

  comments: {
    collection: 'TicketComment',
    via: 'ticket'
  },
}

// TicketComment -------------------------------------------------------------
attributes: {
  id: {
    type: 'string',
    columnName: 'comment_id',
    primaryKey: true
  },
  commentBody: 'string',

  ////////////////////////
  // Associations
  ticket: {
    type: 'string',
    columnName: 'parent_ticket_id',
    model: 'ticket'
  }
}

@particlebanana
Copy link
Contributor

Thanks @jasonsims I'll take a look

@Mordred
Copy link
Contributor Author

Mordred commented Apr 3, 2014

I've tried to debug this problem. I came to the line https://github.com/balderdashy/waterline/blob/master/lib/waterline/query/deferred.js#L121 where

childKey: attr.on

Where the attr.on is come from? I didn't find were this is filled.

I think @jasonsims isn't correct with the line https://github.com/balderdashy/waterline/blob/master/lib/waterline/query/deferred.js#L83 because on the line https://github.com/balderdashy/waterline/blob/master/lib/waterline/query/deferred.js#L119 there is what he suggest.

parentKey: attr.columnName || pk

@jasonsims
Copy link
Contributor

@Mordred deferred.js#L119 parentKey: attr.columnName || pk isn't enough to take care of this. I believe attr.columnName is actually referring to a columnName property for the foreign key (so, parentKey: foreignKey || parentKey). For primary keys, pk is defined on deferred#L83. pk is just being set to the key name of whatever attribute contains the property primaryKey instead of checking for the optional columnName.

There's a separate issue going on which you mentioned with childKey: attr.on. I also could not find where this was being set. It seems that whatever is setting the value here is also ignoring the columnName property though.

@Mordred
Copy link
Contributor Author

Mordred commented Apr 3, 2014

Sorry, you are right

@particlebanana
Copy link
Contributor

Ok guys finally had some time to dig into this and pushed out a fix to Waterline with the PK fix (thanks @jasonsims ) and a patch to Waterline-Schema to fix the attr.on issue.

Using the schema above I tested it and it seems to be good. Would you guys mind pulling down the changes and giving it a go?

@jasonsims
Copy link
Contributor

Sorry for the delay! I got some time to test your fixes out today and it seems partially fixed. The columnName attribute on primary key isn't fully ignored anymore but I'm still seeing an issue with one-to-many relationships. For example, from the schema above, one ticket can have many comments. For some reason the server is returning all items from the many side of the op but when waterline integrates the results, all but one of the items are dropped.

// Expected
{
  id: 1234,
  ticketNumber: "0000013",
  comments: [
    {id: 582834, ticket: 1234, commentBody: "Comment #1"},
    {id: 822242, ticket: 1234, commentBody: "Comment #2"},
    {id: 682499, ticket: 1234, commentBody: "Comment #3"}
}
// Actual
{
  id: 1234,
  ticketNumber: "0000013",
  comments: [
    {id: 582834, ticket: 1234, commentBody: "Comment #1"}
}

@particlebanana did you also test the fix against this one-to-many scenario? The schema above defines the relationship as one-to-many but unless there were multiple objects in the datastore on the many side, this particular issue wouldn't present itself.

particlebanana referenced this issue in balderdashy/waterline Apr 30, 2014
@particlebanana
Copy link
Contributor

Ok @jasonsims just pushed up another fix. Tested it with your data schema and it works. Let me know if you see any more issues.

@jasonsims
Copy link
Contributor

Nice, all is well now. @particlebanana do you know approximately when this will make it into the next RC?

@particlebanana
Copy link
Contributor

I can publish it tomorrow after giving the issues another run through.

@particlebanana
Copy link
Contributor

Ok @jasonsims this is published to npm as 0.10.0-rc11

@johnabrams7 johnabrams7 transferred this issue from balderdashy/waterline May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants