From 0d559ef56a7425051c82e49f7abcdc7e2caf5ec6 Mon Sep 17 00:00:00 2001 From: Geremia Taglialatela Date: Fri, 23 Feb 2024 16:42:02 +0100 Subject: [PATCH] Remove custom TSRange parser This commit removes the custom TSRange parser implemented in Chronomodel in favor of the standard Rails tsrange parser. Implementation: - Keep returning `nil` instead of Infinity values to keep the previous behaviour and allow Rails 7.0 to work (ref: rails/rails#45099) - Check for `Time` because `validity` is a column with `tsrange` type, so that only `Time`, `nil`, and Infinity are supposed to be there - Bump major version because this is a potential breaking change Motivation: - Resolved dependencies: - Ruby 2.6+ now supports open-ended ranges, addressing the limitation in https://bugs.ruby-lang.org/issues/6864 - Rails tsrange stability issues in rails/rails#13793 and rails/rails#14010 - Standardized usage: - Aligns Chronomodel with Rails conventions, improving compatibility and reducing code complexity. - Leverage official API: - Accesses potential future improvements and bug fixes in the standard tsrange. Considerations: - Applications relying on validity.last might require migration strategies due to open-ended ranges. - Utilize Chronomodel's validity and valid_from / valid_to for accurate handling of open-ended ranges. Benefits: - Simplified codebase - Enhanced Rails compatibility - Future-proofed against potential API changes This change strengthens Chronomodel's foundation and paves the way for seamless integration with the wider Ruby and Rails communities. Close #269 Ref: - rails/rails#45099 --- .rubocop_todo.yml | 1 - README.md | 8 --- lib/chrono_model/adapter.rb | 2 - lib/chrono_model/adapter/tsrange.rb | 72 ------------------- .../time_machine/history_model.rb | 13 +++- lib/chrono_model/version.rb | 2 +- 6 files changed, 12 insertions(+), 86 deletions(-) delete mode 100644 lib/chrono_model/adapter/tsrange.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e23a03b..c98c4a6 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -40,7 +40,6 @@ Metrics/PerceivedComplexity: Naming/MethodParameterName: Exclude: - 'lib/chrono_model/adapter/ddl.rb' - - 'lib/chrono_model/adapter/tsrange.rb' - 'lib/chrono_model/time_machine/time_query.rb' # This cop supports unsafe autocorrection (--autocorrect-all). diff --git a/README.md b/README.md index 9bb4ed5..a751955 100644 --- a/README.md +++ b/README.md @@ -328,10 +328,6 @@ Ensure to run the full test suite before pushing. of an object at a specific point in time within the application could lead to inaccuracies. - * Rails 4+ support requires disabling tsrange parsing support, as it - [is broken][r4-tsrange-broken] and [incomplete][r4-tsrange-incomplete] - as of now, mainly due to a [design clash with ruby][pg-tsrange-and-ruby]. - * The triggers and temporal indexes cannot be saved in schema.rb. The AR schema dumper is quite basic, and it isn't (currently) extensible. As we're using many database-specific features, Chronomodel forces the @@ -412,7 +408,6 @@ This software is Made in Italy :it: :smile:. [pg-exclusion-constraints]: https://www.postgresql.org/docs/9.4/sql-createtable.html#SQL-CREATETABLE-EXCLUDE [pg-btree-gist]: https://www.postgresql.org/docs/9.4/btree-gist.html [pg-comment]: https://www.postgresql.org/docs/9.4/sql-comment.html -[pg-tsrange-and-ruby]: https://bugs.ruby-lang.org/issues/6864 [pg-ctes]: https://www.postgresql.org/docs/9.4/queries-with.html [pg-cte-optimization-fence]: https://www.postgresql.org/message-id/201209191305.44674.db@kavod.com [pg-cte-opt-out-fence]: https://www.postgresql.org/message-id/CAHyXU0zpM5+Dsb_pKxDmm-ZoWUAt=SkHHaiK_DBqcmtxTas6Nw@mail.gmail.com @@ -420,9 +415,6 @@ This software is Made in Italy :it: :smile:. [pg-json-func]: https://www.postgresql.org/docs/9.4/functions-json.html [pg-json-opclass]: https://github.com/ifad/chronomodel/blob/master/sql/json_ops.sql -[r4-tsrange-broken]: https://github.com/rails/rails/pull/13793#issuecomment-34608093 -[r4-tsrange-incomplete]: https://github.com/rails/rails/issues/14010 - [cm-readme-sql]: https://github.com/ifad/chronomodel/blob/master/README.sql [cm-timemachine]: https://github.com/ifad/chronomodel/blob/master/lib/chrono_model/time_machine.rb [cm-cte-impl]: https://github.com/ifad/chronomodel/commit/18f4c4b diff --git a/lib/chrono_model/adapter.rb b/lib/chrono_model/adapter.rb index 8d8aca1..259fe91 100644 --- a/lib/chrono_model/adapter.rb +++ b/lib/chrono_model/adapter.rb @@ -7,7 +7,6 @@ require 'chrono_model/adapter/ddl' require 'chrono_model/adapter/indexes' -require 'chrono_model/adapter/tsrange' require 'chrono_model/adapter/upgrade' module ChronoModel @@ -19,7 +18,6 @@ class Adapter < ActiveRecord::ConnectionAdapters::PostgreSQLAdapter include ChronoModel::Adapter::Migrations include ChronoModel::Adapter::DDL include ChronoModel::Adapter::Indexes - include ChronoModel::Adapter::TSRange include ChronoModel::Adapter::Upgrade # The schema holding current data diff --git a/lib/chrono_model/adapter/tsrange.rb b/lib/chrono_model/adapter/tsrange.rb deleted file mode 100644 index e005e7f..0000000 --- a/lib/chrono_model/adapter/tsrange.rb +++ /dev/null @@ -1,72 +0,0 @@ -# frozen_string_literal: true - -module ChronoModel - class Adapter < ActiveRecord::ConnectionAdapters::PostgreSQLAdapter - module TSRange - # HACK: Redefine tsrange parsing support, as it is broken currently. - # - # This self-made API is here because currently AR4 does not support - # open-ended ranges. The reasons are poor support in Ruby: - # - # https://bugs.ruby-lang.org/issues/6864 - # - # and an instable interface in Active Record: - # - # https://github.com/rails/rails/issues/13793 - # https://github.com/rails/rails/issues/14010 - # - # so, for now, we are implementing our own. - # - class Type < ActiveRecord::ConnectionAdapters::PostgreSQL::OID::Range - OID = 3908 - - def cast_value(value) - return if value == 'empty' - return value if value.is_a?(::Array) - - extracted = extract_bounds(value) - - from = Conversions.string_to_utc_time extracted[:from] - to = Conversions.string_to_utc_time extracted[:to] - - [from, to] - end - - def extract_bounds(value) - from, to = value[1..-2].split(',') - - from_bound = - if value[1] == ',' || from == '-infinity' - nil - else - from[1..-2] - end - - to_bound = - if value[-2] == ',' || to == 'infinity' - nil - else - to[1..-2] - end - - { - from: from_bound, - to: to_bound - } - end - end - - def initialize_type_map(m = type_map) - super.tap do - typ = ChronoModel::Adapter::TSRange::Type - oid = typ::OID - - ar_type = type_map.fetch(oid) - cm_type = typ.new(ar_type.subtype, ar_type.type) - - type_map.register_type oid, cm_type - end - end - end - end -end diff --git a/lib/chrono_model/time_machine/history_model.rb b/lib/chrono_model/time_machine/history_model.rb index b76b2ba..fe3e493 100644 --- a/lib/chrono_model/time_machine/history_model.rb +++ b/lib/chrono_model/time_machine/history_model.rb @@ -209,12 +209,21 @@ def record # :nodoc: current_version end + # Return `nil` instead of -Infinity/Infinity to preserve current + # Chronomodel behaviour and avoid failures with Rails 7.0 and + # unbounded time ranges + # + # Check if `begin` and `end` are `Time` because validity is a `tsrange` + # column, so it is either `Time`, `nil`, and in some cases Infinity. + # + # Ref: rails/rails#45099 + # TODO: consider removing when Rails 7.0 support will be dropped def valid_from - validity.first + validity.begin if validity.begin.is_a?(Time) end def valid_to - validity.last + validity.end if validity.end.is_a?(Time) end alias as_of_time valid_to diff --git a/lib/chrono_model/version.rb b/lib/chrono_model/version.rb index 33d68ad..75d64ad 100644 --- a/lib/chrono_model/version.rb +++ b/lib/chrono_model/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module ChronoModel - VERSION = '3.0.1' + VERSION = '4.0.0' end