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

SingularAssociation#find_target uses unordered scoped.first #11774

Closed
UncleGene opened this Issue Aug 6, 2013 · 17 comments

Comments

Projects
None yet
3 participants
@UncleGene

UncleGene commented Aug 6, 2013

Neither MySQL nor PostgreSQL guarantee order for queries without explicit order statement.

Existing code may result in has_one association getting multiple records on replace with newly created association (if scoped.first will return latter one, replace will not happen)

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Aug 16, 2013

The root cause is unordered .first, and this is fixed on 4.0 branch. Still a problem on 3.x

UncleGene commented Aug 16, 2013

The root cause is unordered .first, and this is fixed on 4.0 branch. Still a problem on 3.x

UncleGene pushed a commit to UncleGene/rails that referenced this issue Aug 21, 2013

Eugene Kalenkovich
Add FinderMethods#first default order by PK
Addresses rails#11774 ( unordered #first makes has_one association fragile /
dependent on order of records returned by database )
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

Unfortunately we can't fix this without introducing a non-backward compatible change in 3-2-stable. So I'm closing.

Member

rafaelfranca commented Sep 12, 2013

Unfortunately we can't fix this without introducing a non-backward compatible change in 3-2-stable. So I'm closing.

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

Can you add a little more explanation? Do you really believe that there were clients relying on potentially arbitrary order of values in #first, or on the fact that #last can break?

UncleGene commented Sep 12, 2013

Can you add a little more explanation? Do you really believe that there were clients relying on potentially arbitrary order of values in #first, or on the fact that #last can break?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

There were clients relying on potentially arbitrary order of values in #first. Even in Rails 4 I already saw an issue of people against this change.

Member

rafaelfranca commented Sep 12, 2013

There were clients relying on potentially arbitrary order of values in #first. Even in Rails 4 I already saw an issue of people against this change.

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

It would be fine if AR did not rely on ordered #first semantics. Singular association uses it to replace the old one. Unfortunately there is no clear way (AFAIK) to fix this for the singular association only.

I probably can buy backwards-compatibility reason for not merging ordered #first change, but it means that this particular issue is still a bug.

UncleGene commented Sep 12, 2013

It would be fine if AR did not rely on ordered #first semantics. Singular association uses it to replace the old one. Unfortunately there is no clear way (AFAIK) to fix this for the singular association only.

I probably can buy backwards-compatibility reason for not merging ordered #first change, but it means that this particular issue is still a bug.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

Yes, agree it is still a bug, but it was fixed on 4.0.0. So as we can't fix on 3-2-stable to not add non-backward compatibility I closed this issue.

Member

rafaelfranca commented Sep 12, 2013

Yes, agree it is still a bug, but it was fixed on 4.0.0. So as we can't fix on 3-2-stable to not add non-backward compatibility I closed this issue.

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

Cannot agree. Please disregard "there is no clear way" statement - it is just me not being able to find it. I am not sure that "I do not know how to fix" is a reasonable cause for closing the issue.

UncleGene commented Sep 12, 2013

Cannot agree. Please disregard "there is no clear way" statement - it is just me not being able to find it. I am not sure that "I do not know how to fix" is a reasonable cause for closing the issue.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

I can reopen this issue if you want but I'll not going to fix. Also Rails 3.2.x will be drop bug fixes release in the next month, so this issue will be closed next month if it was not fixed.

Member

rafaelfranca commented Sep 12, 2013

I can reopen this issue if you want but I'll not going to fix. Also Rails 3.2.x will be drop bug fixes release in the next month, so this issue will be closed next month if it was not fixed.

@rafaelfranca rafaelfranca reopened this Sep 12, 2013

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

Thanks for reopening. I'll continue to look at solution (but still hope that somebody more fluent in AR can take a look).
To clarify, the issue affects only a case when singular association is used with create_xxx, because it creates a new record with already set association, and checks for previous one with #first after that.

UncleGene commented Sep 12, 2013

Thanks for reopening. I'll continue to look at solution (but still hope that somebody more fluent in AR can take a look).
To clarify, the issue affects only a case when singular association is used with create_xxx, because it creates a new record with already set association, and checks for previous one with #first after that.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

Maybe we can call the explicit order on this check?

Member

rafaelfranca commented Sep 12, 2013

Maybe we can call the explicit order on this check?

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

The problem is that I cannot find a place where to add this check without affecting other cases - #find_target is where it is applied, but it is used in all other flows too (where again fix may introduce backwards incompatibility)

UncleGene commented Sep 12, 2013

The problem is that I cannot find a place where to add this check without affecting other cases - #find_target is where it is applied, but it is used in all other flows too (where again fix may introduce backwards incompatibility)

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 12, 2013

Member

If the problem is only with has_one we can define a find_target specific for that class.

Member

rafaelfranca commented Sep 12, 2013

If the problem is only with has_one we can define a find_target specific for that class.

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 12, 2013

The big problem are with multiple levels of responsibilities in inheritance chain, where we know what we need on the first step, but can apply order only on the last one

SingularAsociation#create_record (here we know what's going on) =>

SingularAssociation#set_new_record =>

HasOneAssociation#replace =>

Association#load_target =>

SingularAssociation#find_target (here we can apply order)

UncleGene commented Sep 12, 2013

The big problem are with multiple levels of responsibilities in inheritance chain, where we know what we need on the first step, but can apply order only on the last one

SingularAsociation#create_record (here we know what's going on) =>

SingularAssociation#set_new_record =>

HasOneAssociation#replace =>

Association#load_target =>

SingularAssociation#find_target (here we can apply order)

@UncleGene

This comment has been minimized.

Show comment
Hide comment
@UncleGene

UncleGene Sep 15, 2013

I'd like still to make a case for breaking backwards compatibility. Please check https://gist.github.com/UncleGene/6572152

UncleGene commented Sep 15, 2013

I'd like still to make a case for breaking backwards compatibility. Please check https://gist.github.com/UncleGene/6572152

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 16, 2013

Member

I'm sorry but there is no case for breaking backward compatibility in a
stable release unless it is a security issue. Also, the next release of 3.2
will be the last one so, even more, we can't break backward compatibility
since we will not heave another release to fix whatever need to be fixed in
case of unwanted bug
On Sep 15, 2013 2:20 PM, "Eugene Kalenkovich" notifications@github.com
wrote:

I'd like still to make a case for breaking backwards compatibility. Please
check https://gist.github.com/UncleGene/6572152


Reply to this email directly or view it on GitHubhttps://github.com//issues/11774#issuecomment-24475639
.

Member

rafaelfranca commented Sep 16, 2013

I'm sorry but there is no case for breaking backward compatibility in a
stable release unless it is a security issue. Also, the next release of 3.2
will be the last one so, even more, we can't break backward compatibility
since we will not heave another release to fix whatever need to be fixed in
case of unwanted bug
On Sep 15, 2013 2:20 PM, "Eugene Kalenkovich" notifications@github.com
wrote:

I'd like still to make a case for breaking backwards compatibility. Please
check https://gist.github.com/UncleGene/6572152


Reply to this email directly or view it on GitHubhttps://github.com//issues/11774#issuecomment-24475639
.

@zzak

This comment has been minimized.

Show comment
Hide comment
@zzak

zzak Oct 10, 2013

Member

should this be closed then?

Member

zzak commented Oct 10, 2013

should this be closed then?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Oct 10, 2013

Member

Yup. 3.2.15 is on code freeze and it is the last 3.2.x release.

Member

rafaelfranca commented Oct 10, 2013

Yup. 3.2.15 is on code freeze and it is the last 3.2.x release.

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