Fix for broken model.Model#remove by foreign key #379

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@efdi

efdi commented Jun 4, 2013

Try something like:

geddy.model.Post.remove({userId: this.id})

It will break with

/usr/local/lib/node_modules/geddy/node_modules/model/lib/query/query.js:184
          datatype = descr.datatype;
                          ^
TypeError: Cannot read property 'datatype' of undefined

Tracking this down I've found that in geddy/lib/app/index.js there's a hook on 'beforeRemove' event that changes query conditions object adding 'model' and 'event' attributes in there.
The patch is just a simple workaround for this problem.
Not sure of the best way to fix.

@larzconwell

This comment has been minimized.

Show comment Hide comment
@larzconwell

larzconwell Jun 4, 2013

Contributor

Actually in the docs .remove doesn't take queries, only an ID so maybe that has something to do with it?

Contributor

larzconwell commented Jun 4, 2013

Actually in the docs .remove doesn't take queries, only an ID so maybe that has something to do with it?

@efdi

This comment has been minimized.

Show comment Hide comment
@efdi

efdi Jun 4, 2013

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (note casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.

efdi commented Jun 4, 2013

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (note casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.

@ben-ng

This comment has been minimized.

Show comment Hide comment
@ben-ng

ben-ng Jun 4, 2013

Contributor

You can remove multiple items in a single call, pass in an array of IDs. It's in the tests.

On Jun 4, 2013, at 8:39 AM, Alexander Morokhovets notifications@github.com wrote:

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (notice casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.


Reply to this email directly or view it on GitHub.

Contributor

ben-ng commented Jun 4, 2013

You can remove multiple items in a single call, pass in an array of IDs. It's in the tests.

On Jun 4, 2013, at 8:39 AM, Alexander Morokhovets notifications@github.com wrote:

Well, you're probably right. I didn't even consider this.
It's not clear from documentation how to get a db connection to execute raw SQL. And I couldn't get it reading sources.
The only solution left is to async.each(idsCollection, Model.remove)?

It turns out almost every feature of 'model' I'm trying to use is broken or incomplete.
For example, try to use eager loading for fields with non-default names. It will break.
Eager loading for more than one level of associations is not supported.
Define 'bindings': {model: 'UserBinding'} and you will get 'getbindings' and 'addbinding' methods in model (notice casing in names).
I understand that I should report more issues for this. But good report takes time that I don't have after hunting for so many bugs.
Guys, you SHOULD use unit testing. It really helps.


Reply to this email directly or view it on GitHub.

@efdi

This comment has been minimized.

Show comment Hide comment
@efdi

efdi Jun 5, 2013

ben-ng, It would be two calls actually. Get IDs + delete.

efdi commented Jun 5, 2013

ben-ng, It would be two calls actually. Get IDs + delete.

@efdi

This comment has been minimized.

Show comment Hide comment
@efdi

efdi Jun 5, 2013

ben-ng,
your solution is not working in geddy/model + postgresql environment.
Callback after #remove gets this error:

{ [error: syntax error at or near ")"]
  length: 83,
  name: 'error',
  severity: 'ERROR',
  code: '42601',
  detail: undefined,
  hint: undefined,
  position: '67',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'scan.l',
  line: '1001',
  routine: 'scanner_yyerror' }

I've tried both: Model.remove(idsArray) and Model.remove({id: idsArray})

efdi commented Jun 5, 2013

ben-ng,
your solution is not working in geddy/model + postgresql environment.
Callback after #remove gets this error:

{ [error: syntax error at or near ")"]
  length: 83,
  name: 'error',
  severity: 'ERROR',
  code: '42601',
  detail: undefined,
  hint: undefined,
  position: '67',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'scan.l',
  line: '1001',
  routine: 'scanner_yyerror' }

I've tried both: Model.remove(idsArray) and Model.remove({id: idsArray})

@mde

This comment has been minimized.

Show comment Hide comment
@mde

mde Jun 29, 2013

Contributor

Sorry it's taken so long to have a look at this. There seem to be a lot of separate issues here. It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

We do have both unit and integration tests, although we don't have as much coverage as we'd like. There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

As far as the other issues:

-- What does "eager loading for fields with non-default names" mean? Are you talking about named associations?
-- Eager loading multiple levels of association -- I've opened an issue: mde/model#66
-- I don't understand what you mean by "define 'bindings'" -- are you talking about adding an association for a model definition?

Contributor

mde commented Jun 29, 2013

Sorry it's taken so long to have a look at this. There seem to be a lot of separate issues here. It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

We do have both unit and integration tests, although we don't have as much coverage as we'd like. There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

As far as the other issues:

-- What does "eager loading for fields with non-default names" mean? Are you talking about named associations?
-- Eager loading multiple levels of association -- I've opened an issue: mde/model#66
-- I don't understand what you mean by "define 'bindings'" -- are you talking about adding an association for a model definition?

@efdi

This comment has been minimized.

Show comment Hide comment
@efdi

efdi Jun 29, 2013

It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

I'm not sure of what you mean. If "realtime code" == "code executed on client" then no it's a server side controller action code.

There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

Well when I was elaborating on this bug It was clear from the source code that it would break.
I've ended up using async.each(ids, geddy.model.myModel.remove). Did not try reproduce it since.

What does "eager loading for fields with non-default names" mean? Are you talking about named associations?

It turned out that bug occurs when you try to use associations with compund class names, e.g:

Server = ->
  hasMany "DbConfigs"

geddy.model.Server.first {includes: "dbConfigs"}, (err, server) =>
   console.log server.dbConfigs #=> undefined

geddy.model.Server.first {includes: "db_configs"}, (err, server) => # snake case
   console.log server.db_configs #=> actual array of configs, snake case only as well

I don't understand what you mean by "define 'bindings'"

User = ->
  hasMany 'bindings', model: 'UserBinding'

geddy.model.User.first {}, (err, user) =>
  user.addBinding geddy.model.UserBinding.create(blah: "blah") #=> addBinding is not a function

geddy.model.User.first {}, (err, user) =>
  user.addbinding geddy.model.UserBinding.create(blah: "blah") #=> OK

Thanks for your response!

efdi commented Jun 29, 2013

It looks like from your patch that the breakage happens only in the realtime code. Is that correct?

I'm not sure of what you mean. If "realtime code" == "code executed on client" then no it's a server side controller action code.

There's a shared integration test for removing by an ID, or an array of IDs, and they pass in Postgres.

Well when I was elaborating on this bug It was clear from the source code that it would break.
I've ended up using async.each(ids, geddy.model.myModel.remove). Did not try reproduce it since.

What does "eager loading for fields with non-default names" mean? Are you talking about named associations?

It turned out that bug occurs when you try to use associations with compund class names, e.g:

Server = ->
  hasMany "DbConfigs"

geddy.model.Server.first {includes: "dbConfigs"}, (err, server) =>
   console.log server.dbConfigs #=> undefined

geddy.model.Server.first {includes: "db_configs"}, (err, server) => # snake case
   console.log server.db_configs #=> actual array of configs, snake case only as well

I don't understand what you mean by "define 'bindings'"

User = ->
  hasMany 'bindings', model: 'UserBinding'

geddy.model.User.first {}, (err, user) =>
  user.addBinding geddy.model.UserBinding.create(blah: "blah") #=> addBinding is not a function

geddy.model.User.first {}, (err, user) =>
  user.addbinding geddy.model.UserBinding.create(blah: "blah") #=> OK

Thanks for your response!

@mde

This comment has been minimized.

Show comment Hide comment
@mde

mde Jun 29, 2013

Contributor

All right, so I've pushed a Geddy update that fixes that problem of reference pollution in app/index.js. This is on NPM, v0.8.12. Let me know if this fixes your problem removing associated items by their foreign key.

For future reference, the remove method in the Postgres adapter simply uses the same query transformation as it does for find and update, so you can remove by id, array or ids, or any query criteria you want: https://github.com/mde/model/blob/master/lib/adapters/sql/postgres.js#L266

The problem with the 'includes' query condition has an (already fixed) issue here: mde/model#68

I'm going to have to dig a little more on the issue you're having with 'addBinding' -- we have tests that cover this (using 'addProfile'), and they work correctly.

Contributor

mde commented Jun 29, 2013

All right, so I've pushed a Geddy update that fixes that problem of reference pollution in app/index.js. This is on NPM, v0.8.12. Let me know if this fixes your problem removing associated items by their foreign key.

For future reference, the remove method in the Postgres adapter simply uses the same query transformation as it does for find and update, so you can remove by id, array or ids, or any query criteria you want: https://github.com/mde/model/blob/master/lib/adapters/sql/postgres.js#L266

The problem with the 'includes' query condition has an (already fixed) issue here: mde/model#68

I'm going to have to dig a little more on the issue you're having with 'addBinding' -- we have tests that cover this (using 'addProfile'), and they work correctly.

@mde

This comment has been minimized.

Show comment Hide comment
@mde

mde Jul 1, 2013

Contributor

Your problem with your 'Binding' association is that you lowercased the name in the hasMany call. The current code expected a model definition name (i.e., the constructor). I've updated the code to normalize this better. Fix is in 15c506dcf2ff200c6edeae082213247929a6a765.

Contributor

mde commented Jul 1, 2013

Your problem with your 'Binding' association is that you lowercased the name in the hasMany call. The current code expected a model definition name (i.e., the constructor). I've updated the code to normalize this better. Fix is in 15c506dcf2ff200c6edeae082213247929a6a765.

@mde mde closed this Jul 1, 2013

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