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

When duping an object with translations, also dup the translations #84

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@pwim
Collaborator

pwim commented Sep 8, 2017

I noticed mobility doesn't support duping translations like globalize does.

I took a stab at implementing it, but not sure if this is the right approach.

Also, it wasn't clear for me where to put the test. I'm assuming you want to test something like this against every backend. Where should I add real tests?

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 8, 2017

Owner

Thanks for the PR! You're right this is not a case that is currently handled, and the way you've done it using write/read makes sense.

Regarding specs, there is a (backend-independent) duplication spec, I think it could go there first. In principle it shouldn't need to be tested against every backend since it's just built up from reads and writes, but need to think a bit more about that. To test against all backends, it would need to go in the shared examples (see /spec/support/shared_examples/).

Owner

shioyama commented Sep 8, 2017

Thanks for the PR! You're right this is not a case that is currently handled, and the way you've done it using write/read makes sense.

Regarding specs, there is a (backend-independent) duplication spec, I think it could go there first. In principle it shouldn't need to be tested against every backend since it's just built up from reads and writes, but need to think a bit more about that. To test against all backends, it would need to go in the shared examples (see /spec/support/shared_examples/).

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 8, 2017

Owner

I'll have a more careful look later today.

Owner

shioyama commented Sep 8, 2017

I'll have a more careful look later today.

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 8, 2017

Collaborator

Thinking about it some more, this handling is not necessarily needed. For backends where the translation is stored in the model itself (e.g., translatable columns), I don't think anything needs to be done, as the model's dup should take care of it. I don't think what I'm doing will do any harm, but I think the implications are worth thinking about.

Collaborator

pwim commented Sep 8, 2017

Thinking about it some more, this handling is not necessarily needed. For backends where the translation is stored in the model itself (e.g., translatable columns), I don't think anything needs to be done, as the model's dup should take care of it. I don't think what I'm doing will do any harm, but I think the implications are worth thinking about.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 8, 2017

Owner

Yes I agree, which is why I need to think about it. I feel like it shouldn't be the default, maybe somehow as an option, but not sure how that would fit naturally.

Owner

shioyama commented Sep 8, 2017

Yes I agree, which is why I need to think about it. I feel like it shouldn't be the default, maybe somehow as an option, but not sure how that would fit naturally.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 8, 2017

Owner

I thought about this, and in the end if something like this is going to be added, it would have to be a backend-specific thing (done in the setup block for each backend). So e.g.:

setup do |attributes, options|
  # ...

  dup_method = Module.new do
    define_method :initialize_dup do |other|
      # ... do something special applied to attributes
      super
    end
  end
  include dup_method
end

This way only backends where it's relevant to duplicate values would do it, and since it's backend-specific it could bypass read/write to say duplicate associations (in the case of table/key_value backends), etc.

I'm still not sure it makes sense, but that's the way I would probably envision doing it, if it seems important to have.

Owner

shioyama commented Sep 8, 2017

I thought about this, and in the end if something like this is going to be added, it would have to be a backend-specific thing (done in the setup block for each backend). So e.g.:

setup do |attributes, options|
  # ...

  dup_method = Module.new do
    define_method :initialize_dup do |other|
      # ... do something special applied to attributes
      super
    end
  end
  include dup_method
end

This way only backends where it's relevant to duplicate values would do it, and since it's backend-specific it could bypass read/write to say duplicate associations (in the case of table/key_value backends), etc.

I'm still not sure it makes sense, but that's the way I would probably envision doing it, if it seems important to have.

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 9, 2017

Collaborator

I personally don't see the need for initialize_dup behaviour to be configurable by the user of the gem, as it makes things more complicated for little gain. Either the gem should handle it, or decide it is up to the consumer of the gem.

The main reason I think this behaviour should be included in mobility is that one of the core goals of the gem is to provide a unified API regardless of the storage backend. However, without mobility handling dup, the API is not consistent across the backends.

The argument against this is that dup normally creates a shallow copy of an object. I think this is part of the reason why ActiveRecord::Core#dup doesn't copy associations, only attributes. Though with AR#dup, it is also more likely that you wouldn't necessarily want to copy associations. For instance, say we have an Event model that has_many :tickets. When duping, we most likely wouldn't want to dup the tickets association, only the properties of the event itself.

However, the documentation of dup notes

This method may have class-specific behavior. If so, that behavior will be documented under the #initialize_copy method of the class.

So it isn't as though there's a strict rule against us handle duping in mobility.

Given all of this, I'd favour adding the behaviour to the backends that don't handle dup properly right now. I agree that by having it be backend specific, we can just duplicate the associated translations, which should minimize the chance of introducing bugs and also avoid any performance issues.

Collaborator

pwim commented Sep 9, 2017

I personally don't see the need for initialize_dup behaviour to be configurable by the user of the gem, as it makes things more complicated for little gain. Either the gem should handle it, or decide it is up to the consumer of the gem.

The main reason I think this behaviour should be included in mobility is that one of the core goals of the gem is to provide a unified API regardless of the storage backend. However, without mobility handling dup, the API is not consistent across the backends.

The argument against this is that dup normally creates a shallow copy of an object. I think this is part of the reason why ActiveRecord::Core#dup doesn't copy associations, only attributes. Though with AR#dup, it is also more likely that you wouldn't necessarily want to copy associations. For instance, say we have an Event model that has_many :tickets. When duping, we most likely wouldn't want to dup the tickets association, only the properties of the event itself.

However, the documentation of dup notes

This method may have class-specific behavior. If so, that behavior will be documented under the #initialize_copy method of the class.

So it isn't as though there's a strict rule against us handle duping in mobility.

Given all of this, I'd favour adding the behaviour to the backends that don't handle dup properly right now. I agree that by having it be backend specific, we can just duplicate the associated translations, which should minimize the chance of introducing bugs and also avoid any performance issues.

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 14, 2017

Collaborator

After experimenting with this some more, it looks like the workaround proposed in the PR behaves a bit differently than anticipated in some circumstances.

In my own application, I'm using the table backend, and working around it by adding the following to any models that use translation:

  def initialize_dup(source)
    super
    self.translations = source.translations.map(&:dup)
  end

So this further cements for me that we should fix this on a per backend basis. Can I update this PR to fix it for the table backend like the above, or are you still wanting to consider how to handle this?

Collaborator

pwim commented Sep 14, 2017

After experimenting with this some more, it looks like the workaround proposed in the PR behaves a bit differently than anticipated in some circumstances.

In my own application, I'm using the table backend, and working around it by adding the following to any models that use translation:

  def initialize_dup(source)
    super
    self.translations = source.translations.map(&:dup)
  end

So this further cements for me that we should fix this on a per backend basis. Can I update this PR to fix it for the table backend like the above, or are you still wanting to consider how to handle this?

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 14, 2017

Owner

@pwim Please go ahead and fix for the table backend, that would be great.

Owner

shioyama commented Sep 14, 2017

@pwim Please go ahead and fix for the table backend, that would be great.

Dup support for backends for table backend
KeyValue backends require more work to get it working properly, so skip
implementing for now.
@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 15, 2017

Collaborator

I've rebased this PR, and implemented dup support for the ActiveRecord Table backend. All the other backends, except the KeyValue backends, seem to work automatically. Surprisingly, even the Sequel Table backend does, so perhaps they have some built in support.

Using the same technique for the KeyValue backend didn't work. I guess because it is setting up multiple associations to the same table. So for now, I've left it unimplemented.

Collaborator

pwim commented Sep 15, 2017

I've rebased this PR, and implemented dup support for the ActiveRecord Table backend. All the other backends, except the KeyValue backends, seem to work automatically. Surprisingly, even the Sequel Table backend does, so perhaps they have some built in support.

Using the same technique for the KeyValue backend didn't work. I guess because it is setting up multiple associations to the same table. So for now, I've left it unimplemented.

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 15, 2017

Owner

Thanks! Will have a look on the weekend.

Owner

shioyama commented Sep 15, 2017

Thanks! Will have a look on the weekend.

@pwim

This comment has been minimized.

Show comment
Hide comment
@pwim

pwim Sep 16, 2017

Collaborator

Ok, I went with your suggestion of defining a constant for the module.

I was wrong to suggest that we could drop the check of whether it is defined as it will produce a already initialized constant warning if the method is called multiple times with the same association name.

Collaborator

pwim commented Sep 16, 2017

Ok, I went with your suggestion of defining a constant for the module.

I was wrong to suggest that we could drop the check of whether it is defined as it will produce a already initialized constant warning if the method is called multiple times with the same association name.

@shioyama shioyama closed this in e803847 Sep 16, 2017

@shioyama

This comment has been minimized.

Show comment
Hide comment
@shioyama

shioyama Sep 16, 2017

Owner

Thanks for the great PR! Just merged it.

Owner

shioyama commented Sep 16, 2017

Thanks for the great PR! Just merged it.

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