Fix "last equality wins" logic in relation merge #7392

Merged
merged 1 commit into from Aug 19, 2012

Conversation

Projects
None yet
5 participants
@ernie
Contributor

ernie commented Aug 19, 2012

This is a real fix (as compared to the band-aid in b127d86), which uses
the recently-added equality methods for ARel nodes. It has the side
benefit of simplifying the merge code a bit.

Fix "last equality wins" logic in relation merge
This is a real fix (as compared to the band-aid in b127d86), which uses
the recently-added equality methods for ARel nodes. It has the side
benefit of simplifying the merge code a bit.

rafaelfranca added a commit that referenced this pull request Aug 19, 2012

Merge pull request #7392 from ernie/real-fix-for-last-equality-wins-i…
…n-merge

Fix "last equality wins" logic in relation merge

@rafaelfranca rafaelfranca merged commit 1dbf1c8 into rails:master Aug 19, 2012

@tenderlove

This comment has been minimized.

Show comment
Hide comment
@tenderlove

tenderlove Aug 20, 2012

Member

💙💜❤️💚💗💛

Member

tenderlove commented Aug 20, 2012

💙💜❤️💚💗💛

@ernie

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Aug 20, 2012

Contributor

😊

Contributor

ernie commented Aug 20, 2012

😊

@tjwallace

This comment has been minimized.

Show comment
Hide comment
@tjwallace

tjwallace Aug 22, 2012

@ernie why is there "last equality wins" logic anyways? If a users generates a query with WHERE column = 1 AND column = 2, why not let it return no records? This is related to my question/bug #6596.

@ernie why is there "last equality wins" logic anyways? If a users generates a query with WHERE column = 1 AND column = 2, why not let it return no records? This is related to my question/bug #6596.

This comment has been minimized.

Show comment
Hide comment
@ernie

ernie Aug 22, 2012

Contributor
Contributor

ernie replied Aug 22, 2012

@the8472

This comment has been minimized.

Show comment
Hide comment
@the8472

the8472 Oct 12, 2012

As i mentioned in issue #3052 the documented of scopes says that

A scope represents a narrowing of a database query

If understood as a set operation then applying a scope should always result in returning subset of the current records. Overwriting equalities on the other hand could even result in a completely disjoint set compared to the original relation.

This could even be considered a security issue if a default scope is set to hide some records which should remain logically deleted unless .unscoped is used explicitly.

It's also quite inconsistent that A.where().where() behaves differently from A.where().merge(A.where()). It's like two assignments to the same hash would have a different result than merging two different hashes.

It's important to note that merge gets called in all kinds of ways that aren't explicit.

A default scope is a very explicit thing though. And I don't like them for anything but global "nobody (not even admins) should see this by default" stuff. And in exactly that case they don't even work, unless i use in() instead of the equality.
For almost any other use case they often just get in the way forcing you to sprinkle your code with .unscoped in too many places instead of just adding one scope in exactly the place you need it.

the8472 commented Oct 12, 2012

As i mentioned in issue #3052 the documented of scopes says that

A scope represents a narrowing of a database query

If understood as a set operation then applying a scope should always result in returning subset of the current records. Overwriting equalities on the other hand could even result in a completely disjoint set compared to the original relation.

This could even be considered a security issue if a default scope is set to hide some records which should remain logically deleted unless .unscoped is used explicitly.

It's also quite inconsistent that A.where().where() behaves differently from A.where().merge(A.where()). It's like two assignments to the same hash would have a different result than merging two different hashes.

It's important to note that merge gets called in all kinds of ways that aren't explicit.

A default scope is a very explicit thing though. And I don't like them for anything but global "nobody (not even admins) should see this by default" stuff. And in exactly that case they don't even work, unless i use in() instead of the equality.
For almost any other use case they often just get in the way forcing you to sprinkle your code with .unscoped in too many places instead of just adding one scope in exactly the place you need it.

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