Re-parse options at end of class definition

[mikwat]

Currently, track_history must be called after all of the fields/relations are defined because it memoizes them at the time of execution. This issue was described in https://github.com/mongoid/mongoid-history/issues/174.

This proposed fix adds a trace point that runs at end of the class definition and re-parses the options, picking up any fields/relations defined after the track_history call.

@dblock @jnfeinstein

[mongoid-bot]

	1 Warning
:warning:	Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#204](https://github.com/mongoid/mongoid-history/pull/204): Re-parse options at end of class definition - [@mikwat](https://github.com/mikwat).

Generated by :no_entry_sign: danger

[coveralls]

Changes Unknown when pulling bcea418e40c95174783469ea2824187be5805c37 on reparse-options into on master.

[coveralls]

Changes Unknown when pulling bcea418e40c95174783469ea2824187be5805c37 on reparse-options into on master.

[coveralls]

Changes Unknown when pulling bcea418e40c95174783469ea2824187be5805c37 on reparse-options into on master.

[coveralls]

Changes Unknown when pulling bcea418e40c95174783469ea2824187be5805c37 on reparse-options into on master.

[mikwat]

@dblock Looking into this, the problem with deferring parsing and only performing it when the trace point hits is that classes can be defined and modified in a number of creative ways. For example, track_history could be called on the class after the class has been defined. track_history could also be called multiple times on a class (this is done all over the test suite).

So although I don't like the idea of parsing the options twice, I don't see a failsafe alternative. What do you think?

[dblock]

But we could parse in the end only via your new method, instead of doing it once when tracker is declared and once in the end, couldn't we?

[mikwat]

@dblock Here's where that approach fails:

class ModelOne
  include Mongoid::Document
  include Mongoid::History::Trackable
  field :foo
end

ModelOne.track_history(on: [:foo])

[dblock]

Write that as a spec, then lets fix it ;)

Currently:

• track_history is invoked inside the class we parse.
• TracePoint.trace(:end) is invoked from the hook, we re-parse.
• track_history is invoked outside, we re-parse.

What you want:

• track_history is invoked inside the class, but TracePoint hasn't been invoked yet, we don't parse.
• TracePoint.trace(:end) is invoked from the hook, we parse.
• track_history is invoked outside, we re-parse.

Just remember the parsing outcome in TracePoint.trace(:end) and don't parse unless it's set in track_history. So something like reparse if parsed?.

[mikwat]

I like you're enthusiasm and persistence @dblock 😄 I'll spend some more time on this when I can.

[coveralls]

Changes Unknown when pulling 61f2965764e4c6dd9125c7bcab900b393097241c on reparse-options into on master.

[coveralls]

Changes Unknown when pulling 61f2965764e4c6dd9125c7bcab900b393097241c on reparse-options into on master.

[mikwat]

PTAL @dblock

I had to update almost all of the specs because Class.new do ... end does not trigger the tracepoint hook. It seems unlikely to me that in practice anyone defines their mongoid document models this way... but we'll need to document this breaking change.

[dblock]

Actually there're entire libraries that define classes dynamically like https://github.com/mongoid/mongoid-collection-snapshot. I think it's kinda a big deal. Any other suggestions?

[mikwat]

In that case, I don't see any way around double-parsing the options in the case where track_history is called from within the class definition.

[dblock]

Let me think about this. Even with double-parsing the scenario if a dynamic class out of order is a problem.

[dblock]

Reimplemented in https://github.com/mongoid/mongoid-history/pull/208, @mikwat please CR?