Skip to content

fix the bug findAndCountAll not working on postgresql.#899

Merged
janmeier merged 6 commits intosequelize:masterfrom
hackwaly:master
Sep 16, 2013
Merged

fix the bug findAndCountAll not working on postgresql.#899
janmeier merged 6 commits intosequelize:masterfrom
hackwaly:master

Conversation

@hackwaly
Copy link
Copy Markdown
Contributor

copy where array, avoid changed by internal.

copy `where` array, avoid changed by internal.
@janmeier
Copy link
Copy Markdown
Member

Could you please provide a test case that needs this? This should be fixed by #884 - which is in master, but not npm, so you might need to update to that

@hackwaly
Copy link
Copy Markdown
Contributor Author

I'm sorry for my careless commits.

@janmeier
Copy link
Copy Markdown
Member

Thanks for the test :)

For some reason travis has not run your commits so I just tried them locally. The same problem is also present for other dialects, so fixing it for postgres only is not enough. I think you should use the optClone method in the count method like ion #884

@hackwaly
Copy link
Copy Markdown
Contributor Author

That's just I want to say.
but, I think it's not best just wrap the caller functions which pass options parameter. Some internal functions has side effect. They may break other internal functions, not only the count or find etc.
These function should not have side effect, they are not control flow or context aware functions.
What do you think about?

@hackwaly
Copy link
Copy Markdown
Contributor Author

Is this ok?

janmeier added a commit that referenced this pull request Sep 16, 2013
fix the bug `findAndCountAll` not working on postgresql.
@janmeier janmeier merged commit 5da0c23 into sequelize:master Sep 16, 2013
@janmeier
Copy link
Copy Markdown
Member

Jup, very o.k. This is actually better than the fix in 884 so I removed that again since your PR covers all cases

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants