Skip to content
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

Versioned associations feature #24

Merged
merged 16 commits into from Mar 28, 2017
Merged

Versioned associations feature #24

merged 16 commits into from Mar 28, 2017

Conversation

@charlie-wasp
Copy link
Collaborator

@charlie-wasp charlie-wasp commented Feb 16, 2017

It's attempt to implement #19

At the time it lacks some test cases, it covers only belongs_to and has_many associations.

And don't pay attention to a numerous commits, I'll squash it into one when code will be cleaned and fixed

@charlie-wasp charlie-wasp force-pushed the associations branch 2 times, most recently from b44b7db to 6de47e0 Feb 16, 2017
expect(old_post.comments.first.content).to eql('My comment')

very_old_post = post.at(time(100))
expect(very_old_post.comments.length).to eql(0)
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 16, 2017

Note the length method usage instead of size. It corresponds to the problem I described in the issue #19.
Unlike size, length method uses load_target, so we can get what we want with it

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Feb 16, 2017

Oops, travis is not satisfied with my code, I'll get to it tomorrow

@@ -50,6 +50,9 @@ def at(ts)

object_at = dup
object_at.apply_diff(version, log_data.changes_to(version: version))
object_at.id = id
Copy link
Owner

@palkan palkan Feb 17, 2017

Looks like that was a bug. Can you, please, add specs for this and, maybe, it would be better to extract this fix into a separate PR to release it ASAP?

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 17, 2017

If we consider, that results of at invocation couldn't access their association, as a bug, then of course I can extract this particular change

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 17, 2017

@palkan I have opened #25

Copy link
Owner

@palkan palkan left a comment

Overall looks good 👍

The only one crucial feature is missing. Consider the example (from http://cultofmartians.com/tasks/logidze-associations.html):

# 2017-01-19
post = Post.create!(post_params)

# 2017-01-22
comment = post.comments.create(body: 'My comment')

# 2017-01-24
comment.update!(body: 'New text')

# смотрим пост до обновления комментария (первый случай)
old_post = post.at('2017-01-23')

# сам пост в этот момент не менялся, но у нас есть ассоциация, которая поменялась
old_post.comments.first #=> 'My comment'

Thus we cannot rely on log_data.current_version.time, we need something like requested_past_time or whatever.

@@ -122,6 +135,14 @@ def apply_diff(version, diff)
self
end

def in_the_past?
Copy link
Owner

@palkan palkan Feb 17, 2017

Let's prefix this method to avoid possible collisions, smth like logidze_past?.

target.map! do |object|
object unless has_logidze? object

object.at(time)
Copy link
Owner

@palkan palkan Feb 17, 2017

We can use at! here too to avoid #dup calls.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 23, 2017

It's strange, but when I use at, test case with empty comments failed. Somehow super return [false] instead of [], so has_logidze? fails. I guess, somehow at! messes up something inside. I can investigate it, if it's important

def load_target
target = super

return target if inversed
Copy link
Owner

@palkan palkan Feb 17, 2017

Can you explain this line?

Also, specs are needed.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 17, 2017

I'll try to explain it with the example:

when we call post.comments, each comment in this association will have its post association initialized as inversed. So I thought, that we don't want to perform our operations in this case. I'll check this more closely during specs writing

Copy link
Owner

@palkan palkan Feb 17, 2017

Yep. But this post object would exactly the object we call association on, so it will be at the correct time. But, nevertheless, we need specs.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 24, 2017

I made a spec for inversed association. Correct me, if I wrong: in case of belongs_to inversed has_many is not populated anyway, so we don't care about it, right? E.g. in pseudo-code

user = article.user
user.articles.first == article # > false

Copy link
Owner

@palkan palkan Feb 25, 2017

Yep

target
end

def has_logidze?(object)
Copy link
Owner

@palkan palkan Feb 17, 2017

Let's add has_logidze? class method in has_logidze.rb to simplify the check.

@charlie-wasp charlie-wasp force-pushed the associations branch 5 times, most recently from 40bbfa4 to 674bf53 Feb 20, 2017
@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Feb 20, 2017

@palkan want to notice, that I introduced a new model called Article in order to fix specs. It's basically the same as the Post, but with the log_data column. I realized, that Post doesn't have it at the start by design, because this model is used to test generators. So for not messing with that, I created a new model. Is it okay?

@palkan
Copy link
Owner

@palkan palkan commented Feb 20, 2017

@charlie-wasp Yep, that's good.

# Return a dirty copy of record at specified time
# If time is less then the first version, then return nil.
# If time is greater then the last version, then return self.
def at(ts)
ts = parse_time(ts)
@logidze_requested_ts = ts
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 23, 2017

@palkan here's how I covered the case, when at doesn't change the object. Can we do better? I am not particularly happy with attr_reader

Copy link
Owner

@palkan palkan Feb 27, 2017

I'd prefer to store this value in log_data object.

association.singleton_class.prepend Logidze::VersionedAssociation
end

association.reload if @logidze_requested_ts
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 23, 2017

It seems, that with this we can use .size!

Copy link
Owner

@palkan palkan Feb 25, 2017

Hm, but we don't want to reload the association on every call. So we have to add another variable to track associations reload (btw, we should track each association separately) to avoid double-reload:

post = Post.find(id)

# first load, without versions
post.comments

post.at!(2.days.ago)

# here we should reload (it would be better to reset, not reload, btw)
post.comments.first

# no reset/reload here
post.comments.second

# with fresh object
post = Post.find(id).at(2.days.ago)

# no reset/reload, 'cause association hasn't been loaded before
post.comments

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 27, 2017

Yes, it was pretty brutal :) I used reload to invoke load_target in case of already loaded before at invocation belongs_to association:

article.user
old_article = article.at(...)
old_article.user # here load_target will not be called, so we won't have versioned user

But I can work around this issue another way, without reload or reset at all. I pushed commit recently with these changes.

But it seems, that there is no clean way to achieve size method working, so I got back to length so far

@@ -13,6 +13,10 @@ module ClassMethods # :nodoc:
def has_logidze
include Logidze::Model
end

def has_logidze?
included_modules.include? Logidze::Model
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 23, 2017

@palkan Is it what your suggestion was about?

Copy link
Owner

@palkan palkan Feb 25, 2017

No. I suggested to add:

def has_logidze?
  true
end

to Logidze::Model::ClassMethods.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 27, 2017

Mmm, I thought about that, but classes without has_logidze wouldn't respond to this method. So we would have to check it beforehand, which not very convenient. Am I missing something?

Copy link
Owner

@palkan palkan Feb 27, 2017

Yep, you're right. Than we can check for has_logidze? method itself too (i.e. respond_to?(:has_logidze?). It's still a little bit faster than checking included modules.

module Logidze
module VersionedAssociation
module SingularAssociation
def reader(force_reload = false)
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 27, 2017

It turned out, in case of singular association we can use reader method to version instead of load_target. It will save us from the problem of already loaded associations, and multiple invocation won't make us suffer, because logidze won't do anything on the similar at calls

Copy link
Owner

@palkan palkan Feb 27, 2017

But we can still use load_target for single association too, can't we? I would prefer to stick with as less monkey-patched method as possible. Thus, if we can handle all cases within load_target, then let's do that.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 28, 2017

Yes, absolutely, but with the load_target only we have to deal with the reset/reload problem

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Mar 2, 2017

@palkan I've got an idea. We override only load_path and association, as it was before my last commit, but in association we add something like this:

if association.is_a?(ActiveRecord::Associations::SingularAssociation) &&
   association.loaded? &&
   !association.inversed
  # we can extract functionality from load_target and use it here
  association.target.at! @logidze_requested_ts
end

What do you think about it? The same level of monkey-patching and no reload/reset confusion :)

Copy link
Owner

@palkan palkan Mar 2, 2017

And how to handle association loading? I.e. post.at(...).user?
How to prevent double-at!?

I have another idea: we can use stale_target? to handle reload. Just:

def stale_target?
  logidze_state? || super
end

def logidze_stale?
  # we can store logidze_requested_ts in the association too
  loaded? && !inversed? && (owner.logidze_requested_ts != logidze_requested_ts)
end

That should work for all kind of associations.

But I found one more problem with collection associations: we have to think about empty?, any?, blank? methods (which use SQL if target has not been loaded) and also update_all, delete_all.
For presence-like checks we can force load target; and I think we should raise an error when a user tries to use update_all / delete_all on the past version.
Potential caveat:

post = Post.find(...)
post.comments.size #=> 2

# rollback and persist
post.undo!

# no raise
post.comments.update_all(...)

Too complicated. We need more specs, first of all)

UPD: one more issue: post.comment_ids.

Maybe it would be easier to write our own CollectionAssociation that mimics the AR one and make it readonly, than to monkey-patch everything.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Mar 3, 2017

That's a lot of information to process :)

  1. And how to handle association loading? I.e. post.at(...).user?
    How to prevent double-at!?

    All double at! wouldn't affect association, because there will be the same timestamp, so at! will just return self without any processing. Or I miss some scenario?

  2. we can use stale_target?

    What advantage does it have over explicit reload or reset call?

  3. I found one more problem with collection associations

    Got it, I will start with specs for all these methods

Issue gone wild, didn't it? :)

Copy link
Owner

@palkan palkan Mar 3, 2017

Issue gone wild, didn't it? :)

Oh, yeah)

stale_target? is a standard AR mechanism to check, whether we need reload or not, and it's used all over the associations codebase. Thus we're unlikely to miss something. And we should not care about #reload ourselves. I hope so, at least.

All double at! wouldn't affect association ...

Ok.

end

module CollectionAssociation
def load_target
Copy link
Collaborator Author

@charlie-wasp charlie-wasp Feb 27, 2017

In case of collection association we use load_target, because it will be used by CollectionProxy object

def logidze_past?
return false unless log_data

time = log_data.current_version.time
Copy link
Owner

@palkan palkan Feb 27, 2017

We should use logidze_requested_ts here

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 6, 2017

Added usage of stale_target? method

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 8, 2017

@palkan I added specs and implementation of presence-like methods. You said

For presence-like checks we can force load target

Did you mean, that we can basically do something like this?

def empty?
  reload
  super
end

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 13, 2017

@palkan what do you think about my last commit? And what have we got on our plate? I feel kind of lost :) What we should do about update_all? Also, we should take care in a similar way about item_ids= method, I suppose.


if target.is_a? Array
target.map! do |object|
object unless object.class.has_logidze?
Copy link
Owner

@palkan palkan Mar 13, 2017

Why do we check for logidze here? We've already checked that in #association method.

Copy link
Collaborator Author

@charlie-wasp charlie-wasp Mar 14, 2017

Not actually, we don't check, whether target class has logidze or not in the #association method :) And now I think, that it would be better to check it there really

Should we also add specs for the case of associations without logidze enabled?

@palkan
Copy link
Owner

@palkan palkan commented Mar 13, 2017

@charlie-wasp Implementing update_all for versioned associations would be a huge pain)
I think, we can start with read-only features (that should be the most popular case).

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 14, 2017

@palkan so we leave update_all and the similar alone or we should implement raising exception on their invocation?

@palkan
Copy link
Owner

@palkan palkan commented Mar 14, 2017

so we leave update_all and the similar alone or we should implement raising exception on their invocation?

Just ignore them. And we should specify in the Readme the list of supported methods and a note telling that other methods may not work with versions.

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 14, 2017

@palkan good. Now can you tell, please, what else should I do, so we can merge this particular PR? As far as I see, only place for storing logidze_requested_ts is not fixed yet

Copy link
Owner

@palkan palkan left a comment

A few more comments

def association(name)
association = super

unless logidze_past? && association.klass.respond_to?(:has_logidze?)
Copy link
Owner

@palkan palkan Mar 14, 2017

We should also check whether VersionedAssociation has been already prepended

def logidze_past?
return false unless @logidze_requested_ts

time = @logidze_requested_ts
Copy link
Owner

@palkan palkan Mar 14, 2017

We don't need this local var at all


return false if target.empty?

owner.logidze_requested_ts != target.first.logidze_requested_ts
Copy link
Owner

@palkan palkan Mar 14, 2017

There can be a case when we revert one of the target records into a previous state:

post = post.at(122)
post.comments.size #=> 2
post.comments.first.at!(100)
post.at!(100)

# this version is for 122 time
post.comments.second

So, it's better to use target.any? { ... } here.


module CollectionAssociation
def ids_reader
reload
Copy link
Owner

@palkan palkan Mar 14, 2017

Shouldn't we check whether association has been loaded? If it was, then we don't need to reload, do we? The same goes for empty?.

# super
# end

# def is_errored
Copy link
Owner

@palkan palkan Mar 14, 2017

Cleanup, please


should_appply_logidze = logidze_past? &&
association.klass.respond_to?(:has_logidze?) &&
!association.singleton_class.include?(Logidze::VersionedAssociation)

Style/MultilineOperationIndentation: Align the operands of an expression in an assignment spanning multiple lines.

association = super

should_appply_logidze = logidze_past? &&
association.klass.respond_to?(:has_logidze?) &&

Style/MultilineOperationIndentation: Align the operands of an expression in an assignment spanning multiple lines.

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 16, 2017

@palkan are there any more changes to be done?

@palkan
Copy link
Owner

@palkan palkan commented Mar 17, 2017

@charlie-wasp

Final stuff:

  • Great Readme entry with all the caveats explained (or maybe event Wiki article)

  • Change log entry

  • It would be better to rename the PR, it's confusing

As it turned out this feature is rather complex and looks error-prone. What do you think about making it optional and disabled by default? And to enable a user must run smth like Logidze.enable_associations_versioning!

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 17, 2017

@palkan

What do you think about making it optional and disabled by default?

Sounds very reasonable, feature seems quite experimental at the time. I'll try to add a switch. How about putting it into initializer file?

Great Readme entry with all the caveats explained (or maybe event Wiki article)

I would prefer Wiki, especially, if this feature will be turned off by default. How can I create wiki page? At the moment Wiki seems to be disabled

@palkan
Copy link
Owner

@palkan palkan commented Mar 17, 2017

@charlie-wasp

At the moment Wiki seems to be disabled

Check again, please. I've added you to collaborators.

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 17, 2017

@palkan all set, can edit wiki, thank you!

expect(old_comment.article.title).to eql('Article')
end
end

Style/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.

@charlie-wasp charlie-wasp changed the title Saving association changes Versioned associations feature Mar 21, 2017
@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 24, 2017

@palkan I wrote a wiki page, and the README and CHANGELOG entries. Also, I made this feature optional and disabled by default

@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 28, 2017

@palkan hello, any updates on this?

palkan
palkan approved these changes Mar 28, 2017
@palkan palkan merged commit 3be54be into palkan:master Mar 28, 2017
3 checks passed
@palkan
Copy link
Owner

@palkan palkan commented Mar 28, 2017

Great work! Thanks!

@charlie-wasp charlie-wasp deleted the associations branch Mar 28, 2017
@charlie-wasp
Copy link
Collaborator Author

@charlie-wasp charlie-wasp commented Mar 28, 2017

@palkan thank you for the opportunity to contribute and your patience, it was a long run 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants