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

Update crutch.rb to allow crutch inter-dependency with efficiency #965

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

akshay58538
Copy link

@akshay58538 akshay58538 commented Sep 9, 2024

Adds support for passing all crutches into a crutch definition, to be able to form a chain of crutches such that a field can depend and load only what it needs, instead of loading multiple objects at times, which might not all be required always.

Earlier:

class MyIndex

    crutch :my_large_crutch do |models|
        model_rel1_ids = models.map { |m| m.rel1_id }
        rel1_objs = <Load objects of Rel1 from Mongo>
        rel2_ids = rel1_objs.map{ |r| r.rel2_id }
        rel2_objs  = <Load objects of Rel2 from Mongo>
        ..... Relations can go any nested levels
        data = {}
        data[:rel1_objs] = <hash_of rel1_objs>
        data[:rel2_objs] = <hash_of rel2_objs>
        data
    end

    field :field_belongs_to_rel1_obj # <also ends up loading rel2 objects in case of partial update>
    field :field_belongs_to_rel2_obj
end

Now:

class MyIndex

    crutch :my_rel1_objs do |models|
         model_rel1_ids = models.map { |m| m.rel1_id }
        rel1_objs = <Load objects of Rel1 from Mongo>
        <hash of rel1_objs>
    end

    crutch :my_rel2_objs do |_, crutches|
         rel1_objs = crutches.my_rel1_objs
         rel2_ids = rel1_objs.map{ |r| r.rel2_id }
        rel2_objs  = <Load objects of Rel2 from Mongo>
        <hash of rel2_objs>
    end
    
    field :field_belongs_to_rel1_obj # <now can depend only on my_rel1_objs crutch, avoiding loading of rel2 objects, in case of a partial update>
    field :field_belongs_to_rel2_obj
end

Before submitting the PR make sure the following are checked:

  • The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the changelog if the new code introduces user-observable changes. See changelog entry format for details.

Adds support for passing all crutches into a crutch definition, to be able to form a chain of crutches such that a field can depend and load only what it needs, instead of loading multiple objects at times, which might not all be required always.
@akshay58538 akshay58538 changed the title Update crutch.rb Update crutch.rb to allow crutch inter-dependency with efficiency Sep 9, 2024
@konalegi
Copy link
Contributor

konalegi commented Oct 8, 2024

Thanks @akshay58538 for your PR

Overall I think looks good, but you miss 2 things

  • Documentation
  • Specs

Thanks!

@konalegi
Copy link
Contributor

konalegi commented Oct 8, 2024

Yeah, and fix CI please (rubocop specifically)

@akshay58538 akshay58538 requested a review from a team as a code owner October 8, 2024 18:32
@konalegi
Copy link
Contributor

hey @akshay58538, by Documentation I've meant updating of the https://github.com/toptal/chewy/blob/master/README.md and do not forget to add changes into https://github.com/toptal/chewy/blob/master/CHANGELOG.md

@akshay-apollo
Copy link

@konalegi Unrelated, but still asking.

Do you know why mongoId adapter was removed in #776 ?

We were trying to update to latest version of chewy and realized much later, that mongoid adapter itself has been dropped from chewy.

Isnt mongo+ES a very standard architecture in many companies (including mine)?

@arinhouck
Copy link

arinhouck commented Oct 18, 2024

Hmm I just came across this issue hoping this was a resolution to a problem I'm having which I noticed when you have more than one crutch defined (eg. crutch a, crutch b). It seems to evaluate both crutches during the update_fields use case (where the update_fields = [:a], i would only expect crutch a to evaluate).

Maybe I'll have to open another issue for this to enable lazy evaluation of crutches.

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

Successfully merging this pull request may close these issues.

4 participants