Skip to content

Avoid n+1 query on Surveys#5813

Closed
armandfardeau wants to merge 4 commits intodecidim:developfrom
armandfardeau:fix/n+1-query-on-surveys
Closed

Avoid n+1 query on Surveys#5813
armandfardeau wants to merge 4 commits intodecidim:developfrom
armandfardeau:fix/n+1-query-on-surveys

Conversation

@armandfardeau
Copy link
Copy Markdown
Contributor

@armandfardeau armandfardeau commented Mar 1, 2020 •
edited
Loading

🎩 What? Why?

We noticed that the survey component was querying the whole answers table when instantiated.

In decidim-forms/app/controllers/decidim/forms/concerns/has_questionnaire.rb:

def show
  @form = form(Decidim::Forms::QuestionnaireForm).from_model(questionnaire)
  render template: "decidim/forms/questionnaires/show"
end

The consequence was the questionnaire answers were queried, and replaced by the questions.
For a 3370 answers survey, the number of queries needed to render the form was:
6486 request -> 52680 ms in local env

After this fix it was:
38 request -> 37 ms in local env

related to:
#5697

📋 Subtasks

  • Add CHANGELOG entry
  • Add tests

📷 Screenshots (optional)

before:
Capture d’écran 2020-03-01 à 20 33 34

After:
image

microstudi
microstudi reviewed Mar 2, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@microstudi microstudi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @armandfardeau .
I'd like to provide more information about the problem here, as I think it may be something to take into account in other situations.

The main problem here is the use of the Rectify::forms object when using the .from_model method. What this method does is to map all the specified attributes into the model, if the model has a a one-to-many relationship (as are the answers for a questionnaire), you've end with a SELECT * FROM answers SQL query (unless is scoped by default I guess).

using the map_model method inside the definition of the Form class doesn't help here because, this method is executed AFTER building the object, so it does not replaces it. You can see the original code of Rectify::Form that does that here: https://github.com/andypike/rectify/blob/6bb7dcb563fccb4dfee44936ebb7b19e11d18473/lib/rectify/build_form_from_model.rb#L8

Finally, I see that the solution proposed here avoids using .from_model so the build mechanism won't be triggered. This causes, however, to extract part of the functionality done in in the QuestionnaireForm class to the controller, making everything a little bit more untidy. Just thinking out-loud, but maybe this could be solved by using another name than :answers for the attributes in the form?

I like the opinions of people like @mrcasals or @tramuntanal here, do you see this as the best option to proceed? I my opinion, this is important because it can be happening in other places of the Decidim as well, and is extremely easy to overlook. Establishing a generic procedure of how to handle this cases would be very helpful.

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Mar 2, 2020

Great catch @armandfardeau 🎉
Just the other day I was reviewing this on #5697

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 2, 2020

@microstudi I was thinking about the answers outside the form and I must admit that I am not satisfied with the solution I came up with, but I had to correct it quickly and I had already taken too much time. Do you have an idea?

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 2, 2020

Also, if anyone understands why the proposal tests don't pass... I'm in.

@microstudi
Copy link
Copy Markdown
Contributor

microstudi commented Mar 2, 2020

@microstudi I was thinking about the answers outside the form and I must admit that I am not satisfied with the solution I came up with, but I had to correct it quickly and I had already taken too much time. Do you have an idea?

My first idea is just not to use the name :answers in the QuestionnaireForm. Maybe we could use another word like :responses, something that it is not in the model referring a relationship. Or maybe we could remove the attribute :answers and just create a method with the same name. Another solution could be to use a "default" scope for the questionnaire model to query only the answers of the user, but this will likely cause other problems.

@microstudi
Copy link
Copy Markdown
Contributor

microstudi commented Mar 2, 2020

Also, if anyone understands why the proposal tests don't pass... I'm in.

could be this? #5812

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 2, 2020

@microstudi I was thinking about the answers outside the form and I must admit that I am not satisfied with the solution I came up with, but I had to correct it quickly and I had already taken too much time. Do you have an idea?

My first idea is just not to use the name :answers in the QuestionnaireForm. Maybe we could use another word like :responses, something that it is not in the model referring a relationship. Or maybe we could remove the attribute :answers and just create a method with the same name. Another solution could be to use a "default" scope for the questionnaire model to query only the answers of the user, but this will likely cause other problems.

Fell free to add this if you want, unfortunately, I don't have time to push further.

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 2, 2020

Also, if anyone understands why the proposal tests don't pass... I'm in.

could be this? #5812

Great catch thanks.

@armandfardeau armandfardeau marked this pull request as ready for review March 2, 2020 12:49
@mrcasals
Copy link
Copy Markdown
Contributor

mrcasals commented Mar 2, 2020

Hi! In case it's helpful, in other projects I've used https://github.com/flyerhzm/bullet to help detect N+1 queries 😄

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 2, 2020

Hi! In case it's helpful, in other projects I've used https://github.com/flyerhzm/bullet to help detect N+1 queries 😄

This one is provided by Query Diet

@microstudi microstudi mentioned this pull request Mar 3, 2020
Merged
1 task
@microstudi
Copy link
Copy Markdown
Contributor

microstudi commented Mar 3, 2020

@armandfardeau I've made another implementation to fix this, I think it is simpler. Would you like to check it out? https://github.com/decidim/decidim/pull/5819/files

@armandfardeau
Copy link
Copy Markdown
Contributor Author

armandfardeau commented Mar 5, 2020

@armandfardeau I've made another implementation to fix this, I think it is simpler. Would you like to check it out? https://github.com/decidim/decidim/pull/5819/files

This is obviously simpler. Let's go with it.

@armandfardeau armandfardeau deleted the fix/n+1-query-on-surveys branch March 5, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@microstudi microstudi microstudi left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@armandfardeau @andreslucena @microstudi @mrcasals