MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs #5047

johnnyshields · 2021-08-20T16:48:10Z

Mongoid::Document.database_field_name does not currently handle "." delimiter char. As an improvement, it should consider embedded doc :store_as and embedded field aliases.

As a positive side-effect of this PR, distinct and pluck (and maybe other methods) now magically work when using embedded field aliases.

This PR also adds Mongoid::Document.aliased_relations which is a map analogous to aliased_fields

This is a precursor to further improvements on "distinct" and "pluck" methods.

TODO Tests:

Test that embedded aliases work on pluck and distinct methods (this PR fixes that)
Add tests for Document.aliased_relations

database_field_name does not currently handle "." delimiter char. As an improvement, it should consider embedded doc "store_as" and embedded field aliases. This is a precursor to further improvements on "distinct" and "pluck" methods.

p-mongo · 2021-08-20T17:07:10Z

Hi Johnny. Are you sure the ticket was created? I don't see any new tickets from you just now.

p-mongo · 2021-08-20T17:12:40Z

I'm not sure you are aware, as of MongoDB 5.0 dots and dollars in field names are now explicitly supported. So, if a user wants to store in a field named a.b, this field name verbatim should generally be used.

Because of this a change to how dots are treated needs to be carefully considered. Can you add a user-facing example of what you are proposing the behavior to be? It doesn't need to be a test case, just something we can refer to for discussion purposes.

johnnyshields · 2021-08-20T17:56:37Z

@p-mongo sure:

Person.where('phones.number' => '+12061112222')

Person.distinct('phones.number')

This works currently, if there are no aliases.

. and $ fields will not change Mongoid core behavior, and this PR is safe merge. In #5051 documentation I explain that . and $ fields behave very specially--they are essentially "hidden" fields that are not visible to the client (i.e. Mongoid) unless you access them with special accessor methods introduced in MongoDB 5.0)

By the way, I raised this ticket as a precursor to fixes for pluck/distinct demongoization. My plan is to demongoize all fields--even embedded doc fields--correctly.

johnnyshields · 2021-08-20T18:01:08Z

About JIRA... I have tried on two different browsers and two different JIRA accounts, and I can't create tickets. Totally bizarre... it looks like the "Create Ticket" form saves when I press "Submit"--the server even returns 200, but the ticket isn't actually created.

johnnyshields · 2021-08-21T10:49:15Z

@p-mongo I've addressed concerns re: . and $ fields here: #5051

In summary, it is not a problem for this PR. Query operators / field names will behave as normal post-MongoDB 5.0, and $ and . are basically "hidden fields" that can only be accessed with special "shadow" operators.

johnnyshields · 2021-09-02T09:30:12Z

@p-mongo can you please confirm your previous comment is no longer a concern?

* master: MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029)

p-mongo · 2021-09-06T00:53:29Z

Hi @johnnyshields ,

I added integration tests and I was able to verify the distinct and pluck behavior changed as you described, for which I added documentation and release notes.

However for the query example I wasn't getting aliases expanded:

Person.where('phones.number' => '+12061112222')

Can you confirm whether the where call is also supposed to expand the aliases?

lib/mongoid/association/macros.rb

johnnyshields · 2021-09-06T03:32:06Z

@p-mongo

Can you confirm whether the where call is also supposed to expand the aliases?

Yes, where and other query methods should expand the nested aliases (anything which expands the first-level alias should also expand nested). We should include that in this PR.

(Or where could be done as a second PR after this one. I haven't looked into how complex the change would be...)

p-mongo · 2021-09-10T04:33:06Z

lib/mongoid/fields.rb

+          "#{segment}.#{relation.klass.database_field_name(remaining)}"
+        else
+          "#{segment}.#{remaining}"
+        end.freeze


What is the purpose of the freeze calls in this method please?

p-mongo · 2021-09-10T04:37:03Z

This PR as far as I can tell is complete with respect to pluck and distinct, including documentation. Therefore my suggestion would be to merge it as is and tackle where in another PR.

For where, while I generally agree that aliased field expansion could be useful, I am wondering if the various mechanisms that Mongoid provides all would work with it (symbol operators, Mongoid methods and MQL provided as hashes).

johnnyshields · 2021-09-10T04:48:28Z

Interpolated strings arent frozen by default.

On Fri, Sep 10, 2021 at 13:33 Oleg Pudeyev ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/mongoid/fields.rb <#5047 (comment)>: > def database_field_name(name) - return nil unless name - normalized = name.to_s - aliased_fields[normalized] || normalized + return nil unless name.present? + key = name.to_s + segment, remaining = key.split('.', 2) + segment = aliased_fields[segment] || segment + return segment.freeze unless remaining + + relation = relations[aliased_relations[segment] || segment] + if relation + "#{segment}.#{relation.klass.database_field_name(remaining)}" + else + "#{segment}.#{remaining}" + end.freeze What is the purpose of the freeze calls in this method please? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5047 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAGYB4W2ZBVZN6MUUMAN5LUBGDA3ANCNFSM5CQVIRYQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Johnny Shields Co-Founder & Chief Technology Officer [image: TableCheck] <https://www.tablecheck.com/en/company/> [image: Address] Dai 27 Chuo Bldg 4F, 2-14-5 Ginza, Chuo-ku, Tokyo, Japan 104-0061 <https://goo.gl/maps/G4PyDSZE8s32> [image: Phone] +81 (0)80-3519-5647 [image: Web] ***@***.*** https://www.tablecheck.com/en/company/

p-mongo · 2021-09-10T04:53:18Z

Thanks for clarifying that. It's a bit hard to tell from the diff but it seems to me that previously the return value of the method was not frozen. Changing it to a frozen string would technically be an API change and we've had an issue in the past with users expecting unfrozen objects from our methods. Is freezing the strings as you originally wrote an optimization only or was it required for functionality?

johnnyshields · 2021-09-10T06:34:44Z

Freezing not required. Let's go with your way.

p-mongo · 2021-09-10T22:41:40Z

https://jira.mongodb.org/browse/MONGOID-5151

comandeo

@p-mongo What is the reason of adding commented out tests, Maybe we can use pending? I executed this file locally, and only test failed.

p-mongo · 2021-09-14T22:14:12Z

I removed it and put it in the ticket for future use, if we end up needing it.

johnnyshields · 2021-09-15T12:01:16Z

Thanks for the merge, I appreciate it!

* master: MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs (mongodb#5047) RUBY-2675 test coverage on the Mongoid side (mongodb#5030) MONGOID-4592 Add examples of using read_attribute and write_attribute to implement custom field behavior (mongodb#5082) MONGOId-5185 Remove tests for _id serialization (mongodb#5081) MONGOID-5103 Implement eq symbol operator (mongodb#5076) Fix MONGOID-5006 Link default auth source documentation to driver instead of incorrectly claiming "admin" is always the default (mongodb#5048) Create security policy following github's template (mongodb#5070)

* master: (26 commits) MONGOID-5128 Scoped associations (mongodb#5017) MONGOID-5005 - .sum, .count, and similar aggregables now ignore sort if not limiting/skipping (mongodb#5049) MONGOID-5098 Improve specs for timezone handling (specs only; no behavior change) (mongodb#5023) MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs (mongodb#5047) RUBY-2675 test coverage on the Mongoid side (mongodb#5030) MONGOID-4592 Add examples of using read_attribute and write_attribute to implement custom field behavior (mongodb#5082) MONGOId-5185 Remove tests for _id serialization (mongodb#5081) MONGOID-5103 Implement eq symbol operator (mongodb#5076) Fix MONGOID-5006 Link default auth source documentation to driver instead of incorrectly claiming "admin" is always the default (mongodb#5048) Create security policy following github's template (mongodb#5070) MONGOID-5131 Use proper mdb <-> ubuntu versions (mongodb#5067) MONGOID-5131: Set up build pipeline for outside contributors (mongodb#5043) MONGOID-5165 Fix documentation link (mongodb#5065) Fix indentation MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029) MONGOID-5177 Fix flaky contextual spec (mongodb#5064) MONGOID-5170 - Fix more typos mostly in code docs and code comments (mongodb#5056) MONGOID-5105 Allow block form in Mongoid::Association::EmbedsMany::Proxy#count (mongodb#5060) MONGOID-5162 Add a release note for the planned changes in ObjectId#as_json (mongodb#5059) MONGOID-5171 - Make the minimum Ruby version 2.5 (mongodb#5058) ...

* master: MONGOID-5128 Scoped associations (mongodb#5017) MONGOID-5005 - .sum, .count, and similar aggregables now ignore sort if not limiting/skipping (mongodb#5049) MONGOID-5098 Improve specs for timezone handling (specs only; no behavior change) (mongodb#5023) MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs (mongodb#5047) RUBY-2675 test coverage on the Mongoid side (mongodb#5030) MONGOID-4592 Add examples of using read_attribute and write_attribute to implement custom field behavior (mongodb#5082) MONGOId-5185 Remove tests for _id serialization (mongodb#5081) MONGOID-5103 Implement eq symbol operator (mongodb#5076) Fix MONGOID-5006 Link default auth source documentation to driver instead of incorrectly claiming "admin" is always the default (mongodb#5048) Create security policy following github's template (mongodb#5070)

p-mongo changed the title ~~MONGOID-???? database_field_name should work with embedded fields~~ MONGOID-5156 database_field_name should work with embedded fields Aug 25, 2021

p-mongo changed the title ~~MONGOID-5156 database_field_name should work with embedded fields~~ MONGOID-5152 MONGOID-5156 database_field_name should work with embedded fields Aug 25, 2021

p-mongo changed the title ~~MONGOID-5152 MONGOID-5156 database_field_name should work with embedded fields~~ MONGOID-5151 MONGOID-5152 MONGOID-5156 database_field_name should work with embedded fields Aug 25, 2021

Merge branch 'master' into db-field-name-embedded

1c7bf15

p added 4 commits September 5, 2021 20:23

Merge branch 'master' into pr-5047

96bc691

* master: MONGOID-5029 #empty method should use an #exists? rather than a #count query (mongodb#5029)

add integration tests

dd41495

add documentation

2401cf6

add release note

3c6e2ea

p-mongo reviewed Sep 6, 2021

View reviewed changes

lib/mongoid/association/macros.rb Outdated Show resolved Hide resolved

comandeo-mongo and others added 3 commits September 9, 2021 12:12

Merge branch 'master' into db-field-name-embedded

3829904

relations -> associations

58f762c

mark private

524bb2e

p-mongo reviewed Sep 10, 2021

View reviewed changes

p added 2 commits September 10, 2021 00:34

replace freezes with dup

b9e7447

Comment out the where test

30d60e7

relations -> associations

638dc5e

p-mongo added the oleg-check label Sep 10, 2021

p-mongo added oleg-done and removed oleg-check labels Sep 10, 2021

p-mongo requested a review from comandeo September 10, 2021 22:38

p-mongo approved these changes Sep 10, 2021

View reviewed changes

p-mongo changed the title ~~MONGOID-5151 MONGOID-5152 MONGOID-5156 database_field_name should work with embedded fields~~ MONGOID-5151 database_field_name should work with embedded fields Sep 10, 2021

p-mongo changed the title ~~MONGOID-5151 database_field_name should work with embedded fields~~ MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs Sep 10, 2021

comandeo approved these changes Sep 13, 2021

View reviewed changes

remove commented out test

aea0c84

p-mongo requested a review from comandeo September 14, 2021 22:15

comandeo approved these changes Sep 15, 2021

View reviewed changes

p-mongo merged commit d7076b6 into mongodb:master Sep 15, 2021

johnnyshields deleted the db-field-name-embedded branch September 15, 2021 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs #5047

MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs #5047

johnnyshields commented Aug 20, 2021 •

edited

Loading

p-mongo commented Aug 20, 2021

p-mongo commented Aug 20, 2021

johnnyshields commented Aug 20, 2021 •

edited

Loading

johnnyshields commented Aug 20, 2021 •

edited

Loading

johnnyshields commented Aug 21, 2021 •

edited

Loading

johnnyshields commented Sep 2, 2021

p-mongo commented Sep 6, 2021

johnnyshields commented Sep 6, 2021 •

edited

Loading

p-mongo Sep 10, 2021

p-mongo commented Sep 10, 2021

johnnyshields commented Sep 10, 2021 via email

p-mongo commented Sep 10, 2021

johnnyshields commented Sep 10, 2021

p-mongo commented Sep 10, 2021

comandeo left a comment

p-mongo commented Sep 14, 2021

johnnyshields commented Sep 15, 2021

MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs #5047

MONGOID-5151 Respect aliased fields in pluck/distinct by having Document.database_field_name recursively consider embedded docs #5047

Conversation

johnnyshields commented Aug 20, 2021 • edited Loading

TODO Tests:

p-mongo commented Aug 20, 2021

p-mongo commented Aug 20, 2021

johnnyshields commented Aug 20, 2021 • edited Loading

johnnyshields commented Aug 20, 2021 • edited Loading

johnnyshields commented Aug 21, 2021 • edited Loading

johnnyshields commented Sep 2, 2021

p-mongo commented Sep 6, 2021

johnnyshields commented Sep 6, 2021 • edited Loading

p-mongo Sep 10, 2021

Choose a reason for hiding this comment

p-mongo commented Sep 10, 2021

johnnyshields commented Sep 10, 2021 via email

p-mongo commented Sep 10, 2021

johnnyshields commented Sep 10, 2021

p-mongo commented Sep 10, 2021

comandeo left a comment

Choose a reason for hiding this comment

p-mongo commented Sep 14, 2021

johnnyshields commented Sep 15, 2021

johnnyshields commented Aug 20, 2021 •

edited

Loading

johnnyshields commented Aug 20, 2021 •

edited

Loading

johnnyshields commented Aug 20, 2021 •

edited

Loading

johnnyshields commented Aug 21, 2021 •

edited

Loading

johnnyshields commented Sep 6, 2021 •

edited

Loading