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

MONGOID-5098 Range.mongoize should support end-less and begin-less ranges #5026

Merged
merged 8 commits into from Jan 21, 2022

Conversation

johnnyshields
Copy link
Contributor

@johnnyshields johnnyshields commented Aug 11, 2021

This PR enables mongoizing/demongoizing beginless/endless ranges, if Ruby kernel supports it.

When attempting to demongoize a beginless/endless range on a Ruby version which doesn't support it, the database Hash object will be return instead of a Range. This is consistent with how Ranges behave when used on dynamic fields.

(This PR is a continuation of work done in #5108)

@johnnyshields johnnyshields changed the title Standardize Range.mongoize MONGOID-5098 Standardize Range.mongoize Aug 11, 2021
@p-mongo
Copy link
Contributor

p-mongo commented Aug 11, 2021

Regarding conversions to UTC, bson-ruby handles local time to UTC conversions during persistence, as documented in https://docs.mongodb.com/ruby-driver/master/tutorials/bson-v4/#time-instances. Is there a deficiency in Mongoid/Ruby driver/bson-ruby that is corrected by performing local time to UTC conversions in Mongoid?

The "endless ranges" piece is interesting, I need to reseacrh this some more before commenting on it.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Aug 12, 2021

@p-mongo Mongoid converts to UTC in all type classes before passing to driver, hence Range should be considered as a bug/missing behavior. See for example:

https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/time.rb#L84
https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/date.rb#L78
https://github.com/mongodb/mongoid/blob/master/lib/mongoid/extensions/array.rb#L60

I'd be fine to remove the UTC conversion logic from all Mongoid extensions (Time, Date, Array, Hash, etc.), but as long as we have it, Range should be consistent. We could merge this PR then do the UTC removal everywhere in a follow-up PR.

@johnnyshields
Copy link
Contributor Author

Please run CI on this an other PRs?

@p-mongo
Copy link
Contributor

p-mongo commented Aug 25, 2021

I read through this PR and it seems to me that there are multiple independent changes being made which makes it difficult to review and comment in this PR.

My general questions at this point are:

min/max of Range hash should be mongoized.

Why? What problem(s) does this solve or what new functionality does this enable?

    # @return [ Range, Hash ] The range, or database hash object if cannot be represented as range.

We do not seem to have any existing documentation on range persistance in https://docs.mongodb.com/mongoid/master/reference/fields/, which does make understanding the proposed changes here more difficult, but is this PR currently going to return a Hash for something that is declared to be a Range? If so I need to think about whether this makes sense. My first reaction is this would move failures downstream where they would be more difficult to diagnose as objects would be of wrong types with no indication at the failure site why they are of the wrong types.

Correct me if I'm wrong, it seems to me that the timezone changes have nothing to do with the endless ranges. Do they go with mongoization or are they completely independent from the other three components altered by this PR?

@johnnyshields
Copy link
Contributor Author

  1. min/max of Range hash should be mongoized. Why? What problem(s) does this solve

Every kernel object should be mongoized, and all its members must be mongoized. This applies to a Hash (and all its nested members), a Array (and all it's nested members), and in this case Range (and its members). Imagine Ruby 4.0 introduces a kernel object called Foobar, we'll need to mongoize it (and its members) too.

Unlike kernel other objects, Range was never implemented properly, and this PR fixes it.

Questions 2. / .3

You are correct that the timezone changes have nothing to with endless ranges. I will split this into two PRs to make it easier to review and we can discuss each separately.

@johnnyshields
Copy link
Contributor Author

johnnyshields commented Sep 25, 2021

Re: demongoization version incompatibility, one thing we could do is convert to (1..Float::INFINITY) on older Ruby versions. This actually works well with the cover? method at least.

(1..Float::INFINITY).cover?(99) #=> true

(Date.yesterday..Float::INFINITY).cover?(Date.today) #=> true

((Time.now - 1.day)..Float::INFINITY).cover?(Time.now) #=> true

@johnnyshields
Copy link
Contributor Author

@p-mongo this PR is pending your review

@johnnyshields
Copy link
Contributor Author

@p-mongo any update on this? Can someone else take it over if you aren't avialable?

@johnnyshields johnnyshields changed the title MONGOID-5098 Standardize Range.mongoize MONGOID-5098 Range.mongoize should support end-less and begin-less ranges Dec 18, 2021
@johnnyshields
Copy link
Contributor Author

@p-mongo how might we move forward on this?

@p-mongo p-mongo requested a review from comandeo January 21, 2022 09:40
@p-mongo p-mongo merged commit 0a65cc0 into mongodb:master Jan 21, 2022
@johnnyshields johnnyshields deleted the range-mongoize branch January 21, 2022 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants